[dpdk-stable] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
Olivier MATZ
olivier.matz at 6wind.com
Fri Sep 1 11:52:17 CEST 2017
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 at 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.
More information about the stable
mailing list