diff mbox

[dpdk-dev] ixgbe: eliminate duplicate filterlist symbols

Message ID a6c56042-0cff-924d-85cd-07e64fe85f5f@intel.com (mailing list archive)
State Not Applicable, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Ferruh Yigit Aug. 28, 2017, 1:26 p.m. UTC 
  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.

something like:

Comments

David Harton Aug. 28, 2017, 5:13 p.m. UTC | #1 
> -----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.

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);

> +
Ferruh Yigit Aug. 29, 2017, 4:23 p.m. UTC | #2 
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.

> 
> 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);
>> +
David Harton Sept. 8, 2017, 12:11 p.m. UTC | #3 
Hi Konstantin,

A gentle reminder to weigh in on this patch and the 
discussion between Ferruh and myself below.

Thanks,
Dave

> -----Original Message-----

> From: David Harton (dharton)

> 

> > -----Original Message-----

> > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]

> >

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

> 

> 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);

> > +
Ferruh Yigit Sept. 14, 2017, 10:10 a.m. UTC | #4 
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);
>>> +
>
David Harton Sept. 14, 2017, 11:40 a.m. UTC | #5 
> -----Original Message-----

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]

> Sent: Thursday, September 14, 2017 6:10 AM

> To: David Harton (dharton) <dharton@cisco.com>;

> konstantin.ananyev@intel.com

> Cc: dev@dpdk.org; Wenzhuo Lu <wenzhuo.lu@intel.com>

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

> symbols

> 

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


Works for me.  Let me make the changes and test.

Regards,
Dave

> 

> 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);

> >>> +

> >
diff mbox

Patch

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);
+