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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.
> 
>>
>> <...>
>>
>>> @@ -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.
>