[dpdk-dev,7/9] net/virtio: rationalize setting of Rx/Tx handlers
Checks
Commit Message
The selection of Rx/Tx handlers is done at several places,
group them in one function set_rxtx_funcs().
The update of hw->use_simple_rxtx is also rationalized:
- initialized to 1 (prefer simple path)
- in dev configure or rx/tx queue setup, if something prevents from
using the simple path, change it to 0.
- in dev start, set the handlers according to hw->use_simple_rxtx.
Cc: stable@dpdk.org
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
drivers/net/virtio/virtio_ethdev.c | 52 +++++++++++++++++++++++++++++---------
drivers/net/virtio/virtio_rxtx.c | 23 +++++------------
2 files changed, 46 insertions(+), 29 deletions(-)
Comments
On Thu, Aug 31, 2017 at 03:40:13PM +0200, Olivier Matz wrote:
> The selection of Rx/Tx handlers is done at several places,
> group them in one function set_rxtx_funcs().
>
> The update of hw->use_simple_rxtx is also rationalized:
> - initialized to 1 (prefer simple path)
> - in dev configure or rx/tx queue setup, if something prevents from
> using the simple path, change it to 0.
> - in dev start, set the handlers according to hw->use_simple_rxtx.
>
> Cc: stable@dpdk.org
This patch looks like a refactoring to me, that I don't think it's really
a good idea to back port it to a stable release.
...
> @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
>
> eth_dev->dev_ops = &virtio_eth_dev_ops;
> - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
>
> if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> if (!hw->virtio_user_dev) {
> @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> }
>
> virtio_set_vtpci_ops(hw);
> - if (hw->use_simple_rxtx) {
> - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> - } else {
> - rx_func_get(eth_dev);
> - }
> + set_rxtx_funcs(eth_dev);
No need to invoke it here?
> +
> return 0;
> }
>
> @@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> return -EBUSY;
> }
>
> + hw->use_simple_rxtx = 1;
> +
> +#if defined RTE_ARCH_X86
> + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
DPDK now requires SSE4.2. You might want to add another patch to remove
this testing.
> + hw->use_simple_rxtx = 0;
> +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> + hw->use_simple_rxtx = 0;
> +#endif
> + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
> + hw->use_simple_rxtx = 0;
> +
> return 0;
> }
>
> @@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
> VIRTQUEUE_DUMP(txvq->vq);
> }
>
> + set_rxtx_funcs(dev);
> hw->started = 1;
>
> /* Initialize Link state */
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index a32e3229f..c9d97b643 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -505,25 +505,14 @@ static void
> virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> const struct rte_eth_txconf *tx_conf)
This function name doesn't quite make sense now after your refactoring.
> {
> - uint8_t use_simple_rxtx = 0;
> struct virtio_hw *hw = dev->data->dev_private;
> + uint8_t use_simple_rxtx = hw->use_simple_rxtx;
>
> -#if defined RTE_ARCH_X86
> - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> - use_simple_rxtx = 1;
> -#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> - use_simple_rxtx = 1;
> -#endif
> - /* Use simple rx/tx func if single segment and no offloads */
> - if (use_simple_rxtx &&
> - (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
> - !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> - PMD_INIT_LOG(INFO, "Using simple rx/tx path");
> - dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> - dev->rx_pkt_burst = virtio_recv_pkts_vec;
> - hw->use_simple_rxtx = use_simple_rxtx;
> - }
> + /* cannot use simple rxtx funcs with multisegs or offloads */
> + if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
> + use_simple_rxtx = 0;
And that's what left in this function. How about just un-folding it?
--yliu
Hi Yuanhan,
On Fri, Sep 01, 2017 at 05:19:16PM +0800, Yuanhan Liu wrote:
> On Thu, Aug 31, 2017 at 03:40:13PM +0200, Olivier Matz wrote:
> > The selection of Rx/Tx handlers is done at several places,
> > group them in one function set_rxtx_funcs().
> >
> > The update of hw->use_simple_rxtx is also rationalized:
> > - initialized to 1 (prefer simple path)
> > - in dev configure or rx/tx queue setup, if something prevents from
> > using the simple path, change it to 0.
> > - in dev start, set the handlers according to hw->use_simple_rxtx.
> >
> > Cc: stable@dpdk.org
>
> This patch looks like a refactoring to me, that I don't think it's really
> a good idea to back port it to a stable release.
I CCed stable for this commit because next ones, which are fixes, depend
on it. If you consider they should not be included in stable, we can also
drop this one.
> ...
> > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> >
> > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> >
> > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > if (!hw->virtio_user_dev) {
> > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > }
> >
> > virtio_set_vtpci_ops(hw);
> > - if (hw->use_simple_rxtx) {
> > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > - } else {
> > - rx_func_get(eth_dev);
> > - }
> > + set_rxtx_funcs(eth_dev);
>
> No need to invoke it here?
I wanted to stay consistent with previous code.
I'm not very used to work with secondary processes, so I'm not 100% it
is ok, but in my understanding, in that case the configuration is done
first by the primary process, and the secondary the pointers to the
rx/tx funcs. I suppose their value can be different than primary ones.
>
> > +
> > return 0;
> > }
> >
> > @@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> > return -EBUSY;
> > }
> >
> > + hw->use_simple_rxtx = 1;
> > +
> > +#if defined RTE_ARCH_X86
> > + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
>
> DPDK now requires SSE4.2. You might want to add another patch to remove
> this testing.
ok, will do.
>
> > + hw->use_simple_rxtx = 0;
> > +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > + hw->use_simple_rxtx = 0;
> > +#endif
> > + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
> > + hw->use_simple_rxtx = 0;
> > +
> > return 0;
> > }
> >
> > @@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > VIRTQUEUE_DUMP(txvq->vq);
> > }
> >
> > + set_rxtx_funcs(dev);
> > hw->started = 1;
> >
> > /* Initialize Link state */
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index a32e3229f..c9d97b643 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -505,25 +505,14 @@ static void
> > virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> > const struct rte_eth_txconf *tx_conf)
>
> This function name doesn't quite make sense now after your refactoring.
It updates hw->use_simple_rxtx, which at the end will change the
rxtx handler. I didn't find a better name.
My other (poor) ideas were:
virtio_update_[rxtx_]handler_requirements
virtio_update_[rxtx_]handler_constraints
virtio_update_[rxtx_]handler_selector
>
> > {
> > - uint8_t use_simple_rxtx = 0;
> > struct virtio_hw *hw = dev->data->dev_private;
> > + uint8_t use_simple_rxtx = hw->use_simple_rxtx;
> >
> > -#if defined RTE_ARCH_X86
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> > - use_simple_rxtx = 1;
> > -#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > - use_simple_rxtx = 1;
> > -#endif
> > - /* Use simple rx/tx func if single segment and no offloads */
> > - if (use_simple_rxtx &&
> > - (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
> > - !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> > - PMD_INIT_LOG(INFO, "Using simple rx/tx path");
> > - dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > - dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > - hw->use_simple_rxtx = use_simple_rxtx;
> > - }
> > + /* cannot use simple rxtx funcs with multisegs or offloads */
> > + if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
> > + use_simple_rxtx = 0;
>
> And that's what left in this function. How about just un-folding it?
>
> --yliu
yep, we can inline it.
On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > >
> > > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > >
> > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > if (!hw->virtio_user_dev) {
> > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > }
> > >
> > > virtio_set_vtpci_ops(hw);
> > > - if (hw->use_simple_rxtx) {
> > > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > - } else {
> > > - rx_func_get(eth_dev);
> > > - }
> > > + set_rxtx_funcs(eth_dev);
> >
> > No need to invoke it here?
>
> I wanted to stay consistent with previous code.
>
> I'm not very used to work with secondary processes, so I'm not 100% it
> is ok, but in my understanding, in that case the configuration is done
> first by the primary process, and the secondary the pointers to the
> rx/tx funcs. I suppose their value can be different than primary ones.
It probably needs some testing, but I think it should be okay, because
the rx/tx funcs will always be updated at dev_start in this patch.
--yliu
On Fri, Sep 01, 2017 at 08:31:06PM +0800, Yuanhan Liu wrote:
> On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > >
> > > > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > > >
> > > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > > if (!hw->virtio_user_dev) {
> > > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > }
> > > >
> > > > virtio_set_vtpci_ops(hw);
> > > > - if (hw->use_simple_rxtx) {
> > > > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > > - } else {
> > > > - rx_func_get(eth_dev);
> > > > - }
> > > > + set_rxtx_funcs(eth_dev);
> > >
> > > No need to invoke it here?
> >
> > I wanted to stay consistent with previous code.
> >
> > I'm not very used to work with secondary processes, so I'm not 100% it
> > is ok, but in my understanding, in that case the configuration is done
> > first by the primary process, and the secondary the pointers to the
> > rx/tx funcs. I suppose their value can be different than primary ones.
>
> It probably needs some testing, but I think it should be okay, because
> the rx/tx funcs will always be updated at dev_start in this patch.
I still have one doubt about this: looking in
examples/multi_process/symmetric_mp/main.c, I can see that rte_eth_dev_start()
is only called on the primary process. So if I remove set_rxtx_funcs() from
eth_virtio_dev_init(), it looks that the rx functions won't be initialized.
Again, I'm not a user of multi_proc, so I may be wrong, but I think
it would be safer to keep it.
Olivier
On Wed, Sep 06, 2017 at 04:40:12PM +0200, Olivier MATZ wrote:
> On Fri, Sep 01, 2017 at 08:31:06PM +0800, Yuanhan Liu wrote:
> > On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > > >
> > > > > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > > > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > > > >
> > > > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > > > if (!hw->virtio_user_dev) {
> > > > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > > }
> > > > >
> > > > > virtio_set_vtpci_ops(hw);
> > > > > - if (hw->use_simple_rxtx) {
> > > > > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > > > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > > > - } else {
> > > > > - rx_func_get(eth_dev);
> > > > > - }
> > > > > + set_rxtx_funcs(eth_dev);
> > > >
> > > > No need to invoke it here?
> > >
> > > I wanted to stay consistent with previous code.
> > >
> > > I'm not very used to work with secondary processes, so I'm not 100% it
> > > is ok, but in my understanding, in that case the configuration is done
> > > first by the primary process, and the secondary the pointers to the
> > > rx/tx funcs. I suppose their value can be different than primary ones.
> >
> > It probably needs some testing, but I think it should be okay, because
> > the rx/tx funcs will always be updated at dev_start in this patch.
>
> I still have one doubt about this: looking in
> examples/multi_process/symmetric_mp/main.c, I can see that rte_eth_dev_start()
> is only called on the primary process. So if I remove set_rxtx_funcs() from
> eth_virtio_dev_init(), it looks that the rx functions won't be initialized.
>
> Again, I'm not a user of multi_proc, so I may be wrong, but I think
> it would be safer to keep it.
Yes, you are right.
--yliu
@@ -1235,14 +1235,36 @@ virtio_interrupt_handler(void *param)
}
+/* set rx and tx handlers according to what is supported */
static void
-rx_func_get(struct rte_eth_dev *eth_dev)
+set_rxtx_funcs(struct rte_eth_dev *eth_dev)
{
struct virtio_hw *hw = eth_dev->data->dev_private;
- if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+
+ if (hw->use_simple_rxtx) {
+ PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
+ eth_dev->data->port_id);
+ eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+ } else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+ PMD_INIT_LOG(INFO,
+ "virtio: using mergeable buffer Rx path on port %u",
+ eth_dev->data->port_id);
eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts;
- else
+ } else {
+ PMD_INIT_LOG(INFO, "virtio: using standard Rx path on port %u",
+ eth_dev->data->port_id);
eth_dev->rx_pkt_burst = &virtio_recv_pkts;
+ }
+
+ if (hw->use_simple_rxtx) {
+ PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
+ eth_dev->data->port_id);
+ eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+ } else {
+ PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
+ eth_dev->data->port_id);
+ eth_dev->tx_pkt_burst = virtio_xmit_pkts;
+ }
}
/* Only support 1:1 queue/interrupt mapping so far.
@@ -1367,8 +1389,6 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
else
eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
- rx_func_get(eth_dev);
-
/* Setting up rx_header size for the device */
if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
@@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
eth_dev->dev_ops = &virtio_eth_dev_ops;
- eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
if (!hw->virtio_user_dev) {
@@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
}
virtio_set_vtpci_ops(hw);
- if (hw->use_simple_rxtx) {
- eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
- eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
- } else {
- rx_func_get(eth_dev);
- }
+ set_rxtx_funcs(eth_dev);
+
return 0;
}
@@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
return -EBUSY;
}
+ hw->use_simple_rxtx = 1;
+
+#if defined RTE_ARCH_X86
+ if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
+ hw->use_simple_rxtx = 0;
+#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+ if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+ hw->use_simple_rxtx = 0;
+#endif
+ if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+ hw->use_simple_rxtx = 0;
+
return 0;
}
@@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
VIRTQUEUE_DUMP(txvq->vq);
}
+ set_rxtx_funcs(dev);
hw->started = 1;
/* Initialize Link state */
@@ -505,25 +505,14 @@ static void
virtio_update_rxtx_handler(struct rte_eth_dev *dev,
const struct rte_eth_txconf *tx_conf)
{
- uint8_t use_simple_rxtx = 0;
struct virtio_hw *hw = dev->data->dev_private;
+ uint8_t use_simple_rxtx = hw->use_simple_rxtx;
-#if defined RTE_ARCH_X86
- if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
- use_simple_rxtx = 1;
-#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
- if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
- use_simple_rxtx = 1;
-#endif
- /* Use simple rx/tx func if single segment and no offloads */
- if (use_simple_rxtx &&
- (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
- !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
- PMD_INIT_LOG(INFO, "Using simple rx/tx path");
- dev->tx_pkt_burst = virtio_xmit_pkts_simple;
- dev->rx_pkt_burst = virtio_recv_pkts_vec;
- hw->use_simple_rxtx = use_simple_rxtx;
- }
+ /* cannot use simple rxtx funcs with multisegs or offloads */
+ if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
+ use_simple_rxtx = 0;
+
+ hw->use_simple_rxtx = use_simple_rxtx;
}
/*