From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
"Tao, Zhe" <zhe.tao@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Richardson, Bruce" <bruce.richardson@intel.com>,
"Chen, Jing D" <jing.d.chen@intel.com>,
"Liang, Cunming" <cunming.liang@intel.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>,
"Zhang, Helin" <helin.zhang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode
Date: Mon, 13 Jun 2016 10:47:35 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725836B6FCC1@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09090348516A@shsmsx102.ccr.corp.intel.com>
Hi Wenzhuo,
> > > > >
> > > > >
> > > > > >
> > > > > > 3. I thought the plan was to introduce a locking in all
> > > > > > appropriate control path functions (dev_start/dev_stop etc.)
> > > > > > Without that locking version of RX/TX seems a bit useless.
> > > > > > Yes, I understand that you do use locking inside dev_reset, but
> > > > > > I suppose the plan was to have a generic solution, no?
> > > > > > Again, interrupt fire when user invokes dev_start/stop or so, so
> > > > > > we still need some synchronisation between them.
> > > > > >
> > > > > > To be more specific, I thought about something like that:
> > > > > >
> > > > > > static inline uint16_t
> > > > > > rte_eth_rx_burst_lock(uint8_t port_id, uint16_t queue_id,
> > > > > > struct rte_mbuf **rx_pkts, const uint16_t nb_pkts) {
> > > > > > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > > > >
> > > > > > #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
> > > > > >
> > > > > > if (queue_id >= dev->data->nb_rx_queues) {
> > > > > > RTE_PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n",
> > queue_id);
> > > > > > return 0;
> > > > > > }
> > > > > > #endif
> > > > > >
> > > > > > + if
> > > > > > + (rte_spinlock_trylock(&dev->data->rx_queue_state[rx_queue_id].
> > > > > > + lock)
> > > > == 0)
> > > > > > + return 0;
> > > > > > + else if (dev->data->rx_queue_state[rx_queue_id] ==
> > > > > > RTE_ETH_QUEUE_STATE_STOPPED)) {
> > > > > > + rte_spinlock_unlock(&dev->data-
> > >rx_queue_state[rx_queue_id].unlock);
> > > > > > + return 0;
> > > > > > +
> > > > > >
> > > > > > nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> > > > > > rx_pkts, nb_pkts);
> > > > > >
> > > > > > + rte_spinlock_unlock(&dev->data-
> > >rx_queue_state[rx_queue_id].un
> > > > > > + lock
> > > > > > + );
> > > > > >
> > > > > > ....
> > > > > >
> > > > > > return nb_rx;
> > > > > > }
> > > > > >
> > > > > > And inside queue_start:
> > > > > >
> > > > > > int
> > > > > > rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t rx_queue_id)
> > {
> > > > > > struct rte_eth_dev *dev;
> > > > > >
> > > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > > > > >
> > > > > > dev = &rte_eth_devices[port_id];
> > > > > > if (rx_queue_id >= dev->data->nb_rx_queues) {
> > > > > > RTE_PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n",
> > > > rx_queue_id);
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start,
> > > > > > -ENOTSUP);
> > > > > >
> > > > > >
> > > > > > rte_spinlock_lock(&dev->data->rx_queue_state[rx_queue_id].lock)
> > > > > I think you add the lock here to stop the rx/tx.
> > > > > But to my opinion, we should lock the rx/tx much earlier before
> > > > > starting the queue. For example, when stop the port, the resource
> > > > > of the
> > > > queues may be released.
> > > >
> > > > I didn't get you here...
> > > > Before releasing the queue resources, queue_stop() has to be executed,
> > right?
> > > Sorry, I saw your example with rte_eth_dev_rx_queue_start, I didn't
> > > know you also want to change rte_eth_dev_rx_queue_stop too.
> > > Agree this should work it we call queue_start/stop when reset the
> > > port. But we will not call them. I find the queue_stop/start are per- queue
> > functions and not supported by all NICs.
> >
> > But right now you do reset only for ixgbe/i40e.
> Not only for ixgbe/i40e. You forget igb, which doesn't support queue_start/stop :)
>
> > For these devices we defiantly do support queue start/stop.
> > And again, it is not only about reset op.
> > If we want to add rx locked (synced), I think it should be in sync with all
> > control API that changes queue state.
> > As I said before it is a lot of work and a lot of hassle...
> > So probably the easiest (and might be safiest) way just leave things as there
> > are right now:
> > we allow user to setup a callback on VF reset, and it is user responsibility to
> > make sure no RX/TX is active while reset operation is performed.
> > Pretty much what Olivier and Stephen suggested, as I understand.
> Agree. It's not a good way to add lock for just one feature. It could be tricky if we want to extend the lock to other features. A whole
> picture is needed.
> We've sent another patch set to let the user setup a callback on VF reset. Depend on that, user can use existing rte APIs to reset the
> VF port. But how about your opinion if we add a specific rte_reset API? It may be easier for the user.
You mean add rte_eth_dev_reset() without any locking inside?
So it when VF reset happens, it would be user responsibility to make sure all IO
over that device is stopped, and then he can call rte_eth_dev_reset(), correct?
Konstantin
next prev parent reply other threads:[~2016-06-13 10:47 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1465278858-5131-1-git-send-email-zhe.tao@intel.com>
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 0/8] support reset of VF link Zhe Tao
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 1/8] lib/librte_ether: support device reset Zhe Tao
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode Zhe Tao
2016-06-07 9:58 ` Ananyev, Konstantin
2016-06-08 7:24 ` Lu, Wenzhuo
2016-06-08 9:19 ` Ananyev, Konstantin
2016-06-12 2:00 ` Lu, Wenzhuo
2016-06-12 23:16 ` Ananyev, Konstantin
2016-06-13 1:06 ` Lu, Wenzhuo
2016-06-13 10:47 ` Ananyev, Konstantin [this message]
2016-06-14 0:42 ` Lu, Wenzhuo
2016-06-14 8:42 ` Ananyev, Konstantin
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 3/8] ixgbe: RX/TX with lock on VF Zhe Tao
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 4/8] ixgbe: implement device reset " Zhe Tao
2016-06-07 10:03 ` Ananyev, Konstantin
2016-06-08 7:24 ` Lu, Wenzhuo
2016-06-08 8:42 ` Ananyev, Konstantin
2016-06-08 9:22 ` Ananyev, Konstantin
2016-06-12 1:03 ` Lu, Wenzhuo
2016-06-12 0:58 ` Lu, Wenzhuo
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 5/8] igb: RX/TX with lock " Zhe Tao
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 6/8] igb: implement device reset " Zhe Tao
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 7/8] i40e: RX/TX with lock " Zhe Tao
2016-06-07 6:53 ` [dpdk-dev] [PATCH v4 8/8] i40e: implement device reset " Zhe Tao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2601191342CEEE43887BDE71AB97725836B6FCC1@irsmsx105.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=bruce.richardson@intel.com \
--cc=cunming.liang@intel.com \
--cc=dev@dpdk.org \
--cc=helin.zhang@intel.com \
--cc=jing.d.chen@intel.com \
--cc=jingjing.wu@intel.com \
--cc=wenzhuo.lu@intel.com \
--cc=zhe.tao@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).