Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist symbols

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

On 8/29/2017 5:23 PM, Ferruh Yigit wrote:
> On 8/28/2017 6:13 PM, David Harton (dharton) wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>> Sent: Monday, August 28, 2017 9:27 AM
>>> To: David Harton (dharton) <dharton@cisco.com>;
>>> konstantin.ananyev@intel.com
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist
>>> symbols
>>>
>>> On 8/25/2017 8:21 PM, David Harton wrote:
>>>> Some compilers generate warnings for duplicate symbols for the set of
>>>> filter lists current defined in ixgbe_ethdev.h.
>>>> This commits moves the defintion and declaration to the source file
>>>> that actually uses them and provides a function to initialize the
>>>> values akin to its flush function.
>>>>
>>>> Signed-off-by: David Harton <dharton@cisco.com>
>>>> ---
>>>>  drivers/net/ixgbe/ixgbe_ethdev.c |  8 ++------
>>>> drivers/net/ixgbe/ixgbe_ethdev.h |  7 +------
>>>>  drivers/net/ixgbe/ixgbe_flow.c   | 18 ++++++++++++++++++
>>>>  3 files changed, 21 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> index 1ec5aaf..ed21af5 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> @@ -1332,12 +1332,8 @@ struct rte_ixgbe_xstats_name_off {
>>>>  	/* initialize l2 tunnel filter list & hash */
>>>>  	ixgbe_l2_tn_filter_init(eth_dev);
>>>>
>>>> -	TAILQ_INIT(&filter_ntuple_list);
>>>> -	TAILQ_INIT(&filter_ethertype_list);
>>>> -	TAILQ_INIT(&filter_syn_list);
>>>> -	TAILQ_INIT(&filter_fdir_list);
>>>> -	TAILQ_INIT(&filter_l2_tunnel_list);
>>>> -	TAILQ_INIT(&ixgbe_flow_list);
>>>> +	/* initialize flow filter lists */
>>>> +	ixgbe_filterlist_init();
>>>
>>> Will it work if list initialization converted to the static
>>> initialization? That would remove requirement of this function call.
>>
>> I was trying to keep the behavior the same:
>>   .probe -> eth_ixgbe_dev_init -> ixgbe_filterlist_init
>>   .remove -> eth_ixgbe_dev_uninit -> ixgbe_filterlist_flush
>>
>> While I think the below would work I always like mirror/parallel relationships like ctor/dtor for objects.
> 
> I would agree if ixgbe_filterlist_init() initializes something more than
> list itself, but for this case initialization can be avoided completely
> which I would prefer.
> 
> Driver maintainer also cc'ed for more comment.

It seems there is no more comment. So, I suggest lets focus on your fix,
my suggestion to convert to static initialization can be done later if
desired.

Still, what do you think about moving declaration of structs as well to
the ixgbe_flow.c? After your change, only ixgbe_flow.c will be using
them, no need to keep them in ixgbe_ethdev.h.

Thanks,
ferruh

> 
>>
>> Is that reasonable?
>>
>> Thanks,
>> Dave
>>
>>>
>>> something like:
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_flow.c
>>> b/drivers/net/ixgbe/ixgbe_flow.c index c8645f056..6184eb4af 100644
>>> --- a/drivers/net/ixgbe/ixgbe_flow.c
>>> +++ b/drivers/net/ixgbe/ixgbe_flow.c
>>> @@ -79,6 +79,24 @@
>>>  #define IXGBE_MAX_N_TUPLE_PRIO 7
>>>  #define IXGBE_MAX_FLX_SOURCE_OFF 62
>>>
>>> +static TAILQ_HEAD(ixgbe_ntuple_filter_list, ixgbe_ntuple_filter_ele)
>>> +       filter_ntuple_list = TAILQ_HEAD_INITIALIZER(filter_ntuple_list);
>>> +
>>> +static TAILQ_HEAD(ixgbe_ethertype_filter_list,
>>> ixgbe_ethertype_filter_ele)
>>> +       filter_ethertype_list =
>>> TAILQ_HEAD_INITIALIZER(filter_ethertype_list);
>>> +
>>> +static TAILQ_HEAD(ixgbe_syn_filter_list, ixgbe_eth_syn_filter_ele)
>>> +       filter_syn_list = TAILQ_HEAD_INITIALIZER(filter_syn_list);
>>> +
>>> +static TAILQ_HEAD(ixgbe_fdir_rule_filter_list, ixgbe_fdir_rule_ele)
>>> +       filter_fdir_list = TAILQ_HEAD_INITIALIZER(filter_fdir_list);
>>> +
>>> +static TAILQ_HEAD(ixgbe_l2_tunnel_filter_list,
>>> ixgbe_eth_l2_tunnel_conf_ele)
>>> +       filter_l2_tunnel_list =
>>> TAILQ_HEAD_INITIALIZER(filter_l2_tunnel_list);
>>> +
>>> +static TAILQ_HEAD(ixgbe_flow_mem_list, ixgbe_flow_mem)
>>> +       ixgbe_flow_list = TAILQ_HEAD_INITIALIZER(ixgbe_flow_list);
>>> +
>