From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 477957CB5; Fri, 1 Sep 2017 11:52:27 +0200 (CEST) Received: from lfbn-1-18623-73.w90-103.abo.wanadoo.fr ([90.103.154.73] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dnii4-0002Qj-Pr; Fri, 01 Sep 2017 11:57:58 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 01 Sep 2017 11:52:17 +0200 Date: Fri, 1 Sep 2017 11:52:17 +0200 From: Olivier MATZ To: Yuanhan Liu Cc: dev@dpdk.org, maxime.coquelin@redhat.com, stephen@networkplumber.org, stable@dpdk.org Message-ID: <20170901095216.kqx7lwdiktq7y5zs@neon> References: <20170831134015.1383-1-olivier.matz@6wind.com> <20170831134015.1383-8-olivier.matz@6wind.com> <20170901091916.GT9736@yliu-home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170901091916.GT9736@yliu-home> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Sep 2017 09:52:27 -0000 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.