From: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@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: Wed, 8 Jun 2016 07:24:01 +0000 [thread overview]
Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC090903483A2C@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6C44E@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, June 7, 2016 5:59 PM
> To: Tao, Zhe; dev@dpdk.org
> Cc: Lu, Wenzhuo; Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing;
> Zhang, Helin
> Subject: RE: [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode
>
>
> Hi Zhe & Wenzhuo,
>
> Please find my comments below.
> BTW, for clarification - is that patch for 16.11?
> I believe it's too late to introduce such significant change in 16.07.
> Thanks
> Konstantin
Thanks for the comments.
Honestly, our purpose is 16.07. Realizing the big impact, we use NEXT_ABI to comment our change. So, I think although we want to merge it in 16.07 this change will become effective after we remove NEXT_ABI in 16.11.
>
> > Define lock mode for RX/TX queue. Because when resetting the device we
> > want the resetting thread to get the lock of the RX/TX queue to make
> > sure the RX/TX is stopped.
> >
> > Using next ABI macro for this ABI change as it has too much impact. 7
> > APIs and 1 global variable are impacted.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: Zhe Tao <zhe.tao@intel.com>
> > ---
> > lib/librte_ether/rte_ethdev.h | 62
> > +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index 74e895f..4efb5e9 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -354,7 +354,12 @@ struct rte_eth_rxmode {
> > jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
> > hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
> > enable_scatter : 1, /**< Enable scatter packets rx handler */
> > +#ifndef RTE_NEXT_ABI
> > enable_lro : 1; /**< Enable LRO */
> > +#else
> > + enable_lro : 1, /**< Enable LRO */
> > + lock_mode : 1; /**< Using lock path */
> > +#endif
> > };
> >
> > /**
> > @@ -634,11 +639,68 @@ struct rte_eth_txmode {
> > /**< If set, reject sending out tagged pkts */
> > hw_vlan_reject_untagged : 1,
> > /**< If set, reject sending out untagged pkts */
> > +#ifndef RTE_NEXT_ABI
> > hw_vlan_insert_pvid : 1;
> > /**< If set, enable port based VLAN insertion */
> > +#else
> > + hw_vlan_insert_pvid : 1,
> > + /**< If set, enable port based VLAN insertion */
> > + lock_mode : 1;
> > + /**< If set, using lock path */
> > +#endif
> > };
> >
> > /**
> > + * The macros for the RX/TX lock mode functions */ #ifdef
> > +RTE_NEXT_ABI #define RX_LOCK_FUNCTION(dev, func) \
> > + (dev->data->dev_conf.rxmode.lock_mode ? \
> > + func ## _lock : func)
> > +
> > +#define TX_LOCK_FUNCTION(dev, func) \
> > + (dev->data->dev_conf.txmode.lock_mode ? \
> > + func ## _lock : func)
> > +#else
> > +#define RX_LOCK_FUNCTION(dev, func) func
> > +
> > +#define TX_LOCK_FUNCTION(dev, func) func #endif
> > +
> > +/* Add the lock RX/TX function for VF reset */ #define
> > +GENERATE_RX_LOCK(func, nic) \ uint16_t func ## _lock(void *rx_queue,
> > +\
> > + struct rte_mbuf **rx_pkts, \
> > + uint16_t nb_pkts) \
> > +{ \
> > + struct nic ## _rx_queue *rxq = rx_queue; \
> > + uint16_t nb_rx = 0; \
> > + \
> > + if (rte_spinlock_trylock(&rxq->rx_lock)) { \
> > + nb_rx = func(rx_queue, rx_pkts, nb_pkts); \
> > + rte_spinlock_unlock(&rxq->rx_lock); \
> > + } \
> > + \
> > + return nb_rx; \
> > +}
> > +
> > +#define GENERATE_TX_LOCK(func, nic) \ uint16_t func ## _lock(void
> > +*tx_queue, \
> > + struct rte_mbuf **tx_pkts, \
> > + uint16_t nb_pkts) \
> > +{ \
> > + struct nic ## _tx_queue *txq = tx_queue; \
> > + uint16_t nb_tx = 0; \
> > + \
> > + if (rte_spinlock_trylock(&txq->tx_lock)) { \
> > + nb_tx = func(tx_queue, tx_pkts, nb_pkts); \
> > + rte_spinlock_unlock(&txq->tx_lock); \
> > + } \
> > + \
> > + return nb_tx; \
> > +}
>
> 1. As I said in off-line dicussiion, I think this locking could (and I think better be)
> impelented completely on rte_ethdev layer.
> So actual PMD code will be unaffected.
> Again that avoids us to introduce _lock version of every RX/Tx function in each
> PMD.
One purpose of implementing the lock in PMD layer is to avoid ABI change. But we introduce the field lock_mode in struct rte_eth_rx/txmode. So seems it's not a good reason now :)
The other purpose is we want to add a lock for every queue. But in rte layer the queue is void *, so we add the lock in the specific structures of the NICs. But as you mentioned below, we can add the lock as dev->data->rx_queue_state it the struct rte_eth_dev_data.
So, I prefer to add the lock in rte layer now.
>
> 2. Again, as discussed offline, I think it is better to have an explicit
> rte_eth_(rx|tx)_burst_lock(sync?) API, instead of add new fileds into RX/TX
> config strcutures.
> Would help to avoid any confusion, I think.
We want the users to choose the rx/tx path without lock if they're sensitive to the performance and can handle the reset event in their APP. After introducing new fields of config struct, users can change the config to choose the different path.
If we introduce new API, it may be harder for the use to use it. I mean when users want to use lock mode, they may need to replace all the rte_eth_rx/tx_burst by rte_eth_rx/tx_burst_lock. So if we add the lock in rte layer, I still prefer adding lock_mode in the configuration, and the rte_eth_rx/tx_burst is changed like this,
rte_eth_rx/tx_burst
{
+ if lock_mode
+ try_lock
......
+ if lock_mode
+ release_lock
}
>
> 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].unlock);
>
> ....
>
> 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. The rx/tx cannot be executed. So I prefer to get the lock before stopping the ports. Maybe better to keep the spinlock in the dev_reset.
>
> if (dev->data->rx_queue_state[rx_queue_id] !=
> RTE_ETH_QUEUE_STATE_STOPPED) {
> RTE_PMD_DEBUG_TRACE("Queue %" PRIu16" of device with
> port_id=%" PRIu8
> " already started\n",
> rx_queue_id, port_id);
> ret = -EINVAL 0;
> } else
> ret = dev->dev_ops->rx_queue_start(dev, rx_queue_id);
>
> rte_spinlock_unlock(&dev->data->rx_queue_state[rx_queue_id].unlock);
>
> return ret;
> }
>
> Then again, we don't need to do explicit locking inside dev_reset().
> Does it make sense to you guys?
Please see the answer above.
>
>
> > +
> > +/**
> > * A structure used to configure an RX ring of an Ethernet port.
> > */
> > struct rte_eth_rxconf {
> > --
> > 2.1.4
next prev parent reply other threads:[~2016-06-08 7:24 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 [this message]
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
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=6A0DE07E22DDAD4C9103DF62FEBC090903483A2C@shsmsx102.ccr.corp.intel.com \
--to=wenzhuo.lu@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=konstantin.ananyev@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).