Re: [dpdk-stable] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Yuanhan Liu <yliu@fridaylinux.org>]

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