patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Olivier MATZ <olivier.matz@6wind.com>
To: Yuanhan Liu <yliu@fridaylinux.org>
Cc: dev@dpdk.org, maxime.coquelin@redhat.com,
	stephen@networkplumber.org, stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
Date: Fri, 1 Sep 2017 11:52:17 +0200	[thread overview]
Message-ID: <20170901095216.kqx7lwdiktq7y5zs@neon> (raw)
In-Reply-To: <20170901091916.GT9736@yliu-home>

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.

  reply	other threads:[~2017-09-01  9:52 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20170831134015.1383-1-olivier.matz@6wind.com>
2017-08-31 13:40 ` [dpdk-stable] [PATCH 1/9] net/virtio: revert "do not claim to support LRO" Olivier Matz
2017-08-31 13:40 ` [dpdk-stable] [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
2017-08-31 13:47   ` [dpdk-stable] [dpdk-dev] " Olivier MATZ
2017-08-31 13:40 ` [dpdk-stable] [PATCH 3/9] doc: fix description of L4 Rx checksum offload Olivier Matz
2017-08-31 13:40 ` [dpdk-stable] [PATCH 4/9] net/virtio: fix log levels in configure Olivier Matz
2017-08-31 13:40 ` [dpdk-stable] [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function Olivier Matz
2017-08-31 13:48   ` [dpdk-stable] [dpdk-dev] " Olivier MATZ
2017-08-31 13:40 ` [dpdk-stable] [PATCH 6/9] net/virtio: fix queue setup consistency Olivier Matz
2017-08-31 13:49   ` [dpdk-stable] [dpdk-dev] " Olivier MATZ
2017-08-31 13:40 ` [dpdk-stable] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
2017-09-01  9:19   ` Yuanhan Liu
2017-09-01  9:52     ` Olivier MATZ [this message]
2017-09-01 12:31       ` Yuanhan Liu
2017-09-06 14:40         ` Olivier MATZ
2017-09-07  8:13           ` Yuanhan Liu
2017-08-31 13:40 ` [dpdk-stable] [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
2017-08-31 13:50   ` [dpdk-stable] [dpdk-dev] " Olivier MATZ
2017-09-01  9:25   ` [dpdk-stable] " Yuanhan Liu
2017-09-01  9:58     ` Olivier MATZ
2017-09-01 12:22       ` Yuanhan Liu
2017-08-31 13:40 ` [dpdk-stable] [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested Olivier Matz
2017-08-31 13:51   ` [dpdk-stable] [dpdk-dev] " Olivier MATZ
     [not found] ` <20170907121347.16208-1-olivier.matz@6wind.com>
2017-09-07 12:13   ` [dpdk-stable] [PATCH v2 01/10] net/virtio: revert "do not claim to support LRO" Olivier Matz
2017-09-07 12:13   ` [dpdk-stable] [PATCH v2 02/10] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
2017-09-07 12:13   ` [dpdk-stable] [PATCH v2 03/10] doc: fix description of L4 Rx checksum offload Olivier Matz
2017-09-07 12:13   ` [dpdk-stable] [PATCH v2 04/10] net/virtio: fix log levels in configure Olivier Matz
2017-09-07 12:13   ` [dpdk-stable] [PATCH v2 05/10] net/virtio: fix mbuf port for simple Rx function Olivier Matz
2017-09-07 12:13   ` [dpdk-stable] [PATCH v2 06/10] net/virtio: fix queue setup consistency Olivier Matz
2017-12-06  5:25     ` [dpdk-stable] [dpdk-dev] " Tiwei Bie
2017-12-07 14:14       ` Olivier MATZ
2017-12-08  2:17         ` Tiwei Bie
2018-02-01  3:14     ` Yao, Lei A
2018-02-01  8:27       ` Olivier Matz
2018-02-07  8:31         ` Xu, Qian Q
2018-02-07 22:01           ` Olivier Matz
2018-02-09  5:44             ` Wang, Zhihong
2018-02-09  8:59               ` Maxime Coquelin
2018-02-09  9:40                 ` Maxime Coquelin

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=20170901095216.kqx7lwdiktq7y5zs@neon \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=stable@dpdk.org \
    --cc=stephen@networkplumber.org \
    --cc=yliu@fridaylinux.org \
    /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).