Re: [dpdk-dev] [PATCH v3 3/8] net/ice: support vector SSE in RX

["Lu, Wenzhuo" <wenzhuo.lu@intel.com>]

Hi Ferruh,


> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, March 21, 2019 1:35 AM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 3/8] net/ice: support vector SSE in RX
> 
> On 3/18/2019 1:22 AM, Lu, Wenzhuo wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Saturday, March 16, 2019 1:54 AM
> >> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v3 3/8] net/ice: support vector SSE in
> >> RX
> >>
> >> On 3/15/2019 6:22 AM, Wenzhuo Lu wrote:
> >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -305,6 +305,7 @@ CONFIG_RTE_LIBRTE_ICE_DEBUG_TX=n
> >>> CONFIG_RTE_LIBRTE_ICE_DEBUG_TX_FREE=n
> >>>  CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC=y
> >>>  CONFIG_RTE_LIBRTE_ICE_16BYTE_RX_DESC=n
> >>> +CONFIG_RTE_LIBRTE_ICE_INC_VECTOR=y
> >>
> >> Meson seems setting this config automatically. Do we need this
> >> compile time option at all?
> > It's not for meson build. It's for the traditional build. To my opinion, meson
> build even doesn't use the configure file. It has its own configuration inside.
> 
> I am not saying this is for meson.
> 
> This is a compile time config option, to let user enable/disable VECTOR path
> of the PMD.
> But meson build doesn't get this input from the user and enables it by
> default.
> If enabling it by default works and an acceptable solution, why we are not
> doing same thing for makefile.
> 
> Why not just remove the config option completely and update code as it is
> enabled?
> 
> In this default enabled case, if there is a need to let user disable the vector
> path, which may be a need, why not add a device argument to disable the
> vector path on runtime, I believe this can be done easily by described below.
> 
> What is the benefit of a compile time flag against runtime devargs?
> Why someone would want to remove the all vector path from the binary,
> just to gain a few kilobytes from the final binary?
> But other way around, having the vector path in binary but disable it
> dynamically when needed has advantage of easily enable it back without
> need to recompile when the platform has vector path support.
> 
> >
> >> Would it work if we replace this with a device arg, which can be used
> >> to disable vector path if set, and 'ice_rx_vec_dev_check()' can check it?
> > We've implemented the dynamic selection of vector and normal path.
> Here is the compile setting. In case the user wants to remove the vector code
> thoroughly, so there may be a little performance benefit.
I think you're right. The vector and normal path can be selected automatically. And we do recommend using vector path if it satisfies user's requirement. I'll remove this configuration.

> >
> >>
> >> <...>
> >>
> >>> @@ -0,0 +1,155 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>> + * Copyright(c) 2019 Intel Corporation  */
> >>> +
> >>> +#ifndef _ICE_RXTX_VEC_COMMON_H_
> >>> +#define _ICE_RXTX_VEC_COMMON_H_
> >>> +
> >>> +#include "ice_rxtx.h"
> >>> +
> >>> +static inline uint16_t
> >>> +reassemble_packets(struct ice_rx_queue *rxq, struct rte_mbuf
> **rx_bufs,
> >>> +		   uint16_t nb_bufs, uint8_t *split_flags) {
> >>> +	struct rte_mbuf *pkts[ICE_VPMD_RX_BURST] = {0}; /*finished pkts*/
> >>> +	struct rte_mbuf *start = rxq->pkt_first_seg;
> >>> +	struct rte_mbuf *end =  rxq->pkt_last_seg;
> >>> +	unsigned pkt_idx, buf_idx;
> >> There are checkpatch warnings for using 'unsigned int' instead of
> >> 'unsigned', can you please fix them? There are a few of them.
> > Sure, will fix them.
> >