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: Tue, 14 Jun 2016 08:42:47 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725836B70788@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC0909034859A8@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].lo
> > > > > > > > ck)
> > > > > > > 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
> Yes, that's exactly what I plan to do :)
Sounds reaosanable to me :)
Konstantin
next prev parent reply other threads:[~2016-06-14 8:42 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
2016-06-14 0:42 ` Lu, Wenzhuo
2016-06-14 8:42 ` Ananyev, Konstantin [this message]
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=2601191342CEEE43887BDE71AB97725836B70788@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).