[dpdk-dev] [PATCH] mk: fix acl library static linking

[dpdk-dev] [PATCH] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

Since below commit, ACL library is outside the scope of --whole-archive
and ACL autotest fails.

  RTE>>acl_autotest
  ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
  ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
  Line 1584: SSE classify with zero categories failed!
  Test Failed

This is the result of the linker picking weak over non-weak functions.

Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
---
 mk/rte.app.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 83314ca..7f89fd4 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -76,12 +76,12 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
 _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
 _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
 _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
-_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
 
 _LDLIBS-y += --whole-archive
 
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
 _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
 _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
-- 
2.4.11

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Thomas Monjalon]

2016-06-30 12:10, Sergio Gonzalez Monroy:
> Since below commit, ACL library is outside the scope of --whole-archive
> and ACL autotest fails.
> 
>   RTE>>acl_autotest
>   ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
>   ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
>   Line 1584: SSE classify with zero categories failed!
>   Test Failed
> 
> This is the result of the linker picking weak over non-weak functions.
> 
> Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")
> 
> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

Please could you detail which symbol is missing?

Does it need to be commented in rte.app.mk?
The other libs are in whole-archive to support dlopen of drivers.
But the problem here is not because of a driver use.

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

On 30/06/2016 12:38, Thomas Monjalon wrote:
> 2016-06-30 12:10, Sergio Gonzalez Monroy:
>> Since below commit, ACL library is outside the scope of --whole-archive
>> and ACL autotest fails.
>>
>>    RTE>>acl_autotest
>>    ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
>>    ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
>>    Line 1584: SSE classify with zero categories failed!
>>    Test Failed
>>
>> This is the result of the linker picking weak over non-weak functions.
>>
>> Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")
>>
>> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> Please could you detail which symbol is missing?

It is not missing a symbol, it is picking weak over non-weak symbol.
It just happens that the only other using weak symbols are PMDs and
they are under the scope of --whole-archive already.

> Does it need to be commented in rte.app.mk?
> The other libs are in whole-archive to support dlopen of drivers.
> But the problem here is not because of a driver use.

There seem to be a bunch of libraries under --whole-archive scope that 
are not
PMDs, ie. cfgfile, cmdline...

What is the criteria?

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez Monroy
> Sent: Thursday, June 30, 2016 12:10 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] mk: fix acl library static linking
> 
> Since below commit, ACL library is outside the scope of --whole-archive
> and ACL autotest fails.
> 
>   RTE>>acl_autotest
>   ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
>   ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
>   Line 1584: SSE classify with zero categories failed!
>   Test Failed
> 
> This is the result of the linker picking weak over non-weak functions.
> 
> Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")
> 
> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> ---
>  mk/rte.app.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index 83314ca..7f89fd4 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -76,12 +76,12 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
> 
>  _LDLIBS-y += --whole-archive
> 
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
> --
> 2.4.11

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Thomas Monjalon]

2016-06-30 13:04, Sergio Gonzalez Monroy:
> On 30/06/2016 12:38, Thomas Monjalon wrote:
> > Does it need to be commented in rte.app.mk?
> > The other libs are in whole-archive to support dlopen of drivers.
> > But the problem here is not because of a driver use.
> 
> There seem to be a bunch of libraries under --whole-archive scope that 
> are not
> PMDs, ie. cfgfile, cmdline...
> 
> What is the criteria?

The criteria is a bit vague. We must try to include only libs which can
be used by a driver.
cmdline should probably not be there.
Does it make sense to use cfgfile in a driver? maybe yes.

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

On 30/06/2016 13:44, Thomas Monjalon wrote:
> 2016-06-30 13:04, Sergio Gonzalez Monroy:
>> On 30/06/2016 12:38, Thomas Monjalon wrote:
>>> Does it need to be commented in rte.app.mk?
>>> The other libs are in whole-archive to support dlopen of drivers.
>>> But the problem here is not because of a driver use.
>> There seem to be a bunch of libraries under --whole-archive scope that
>> are not
>> PMDs, ie. cfgfile, cmdline...
>>
>> What is the criteria?
> The criteria is a bit vague. We must try to include only libs which can
> be used by a driver.
> cmdline should probably not be there.
> Does it make sense to use cfgfile in a driver? maybe yes.

So as it is, ACL autotest is broken when building static libs 
(non-combined).
For combined libs we usually wrap libdpdk.a with --whole-archive, thus it is
not an issue.

Just thinking a bit more about the 'dlopen of drivers' case you 
mentioned before,
shouldn't the driver have proper dependencies and therefore need shared 
DPDK libraries?
What does happen if binary/app and driver are built against different 
library versions?
Where does it say that we do support this use case?

Sergio

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Ananyev, Konstantin]

Hi Thomas,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez Monroy
> Sent: Thursday, June 30, 2016 3:02 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mk: fix acl library static linking
> 
> On 30/06/2016 13:44, Thomas Monjalon wrote:
> > 2016-06-30 13:04, Sergio Gonzalez Monroy:
> >> On 30/06/2016 12:38, Thomas Monjalon wrote:
> >>> Does it need to be commented in rte.app.mk?
> >>> The other libs are in whole-archive to support dlopen of drivers.
> >>> But the problem here is not because of a driver use.
> >> There seem to be a bunch of libraries under --whole-archive scope that
> >> are not
> >> PMDs, ie. cfgfile, cmdline...
> >>
> >> What is the criteria?
> > The criteria is a bit vague. We must try to include only libs which can
> > be used by a driver.
> > cmdline should probably not be there.
> > Does it make sense to use cfgfile in a driver? maybe yes.
> 
> So as it is, ACL autotest is broken when building static libs
> (non-combined).
> For combined libs we usually wrap libdpdk.a with --whole-archive, thus it is
> not an issue.
> 
> Just thinking a bit more about the 'dlopen of drivers' case you
> mentioned before,
> shouldn't the driver have proper dependencies and therefore need shared
> DPDK libraries?
> What does happen if binary/app and driver are built against different
> library versions?
> Where does it say that we do support this use case?
> 
> Sergio
> 

So are you going to apply this patch?
Right now acl just can't be used properly in case of static library build.
Thanks
Konstantin

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Thomas Monjalon]

2016-06-30 15:02, Sergio Gonzalez Monroy:
> On 30/06/2016 13:44, Thomas Monjalon wrote:
> > 2016-06-30 13:04, Sergio Gonzalez Monroy:
> >> On 30/06/2016 12:38, Thomas Monjalon wrote:
> >>> Does it need to be commented in rte.app.mk?
> >>> The other libs are in whole-archive to support dlopen of drivers.
> >>> But the problem here is not because of a driver use.
> >> There seem to be a bunch of libraries under --whole-archive scope that
> >> are not
> >> PMDs, ie. cfgfile, cmdline...
> >>
> >> What is the criteria?
> > The criteria is a bit vague. We must try to include only libs which can
> > be used by a driver.
> > cmdline should probably not be there.
> > Does it make sense to use cfgfile in a driver? maybe yes.
> 
> So as it is, ACL autotest is broken when building static libs 
> (non-combined).

I think the --whole-archive option must be set specifically for ACL
with a comment explaining it is required because of weak functions:

# librte_acl needs --whole-archive because of weak functions
_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL) += --whole-archive -lrte_acl --no-whole-archive

> For combined libs we usually wrap libdpdk.a with --whole-archive, thus it is
> not an issue.
> 
> Just thinking a bit more about the 'dlopen of drivers' case you 
> mentioned before,
> shouldn't the driver have proper dependencies and therefore need shared 
> DPDK libraries?

It is possible to build a .so, without any DT_NEEDED entries, which will
find the required symbols in the static linked binary.

> What does happen if binary/app and driver are built against different 
> library versions?

Bad things :)

> Where does it say that we do support this use case?

It is maybe not written. But I know it is used by people wanting to load
some PMD.so on demand while having the rest statically compiled.
I agree it needs to be documented and probably better managed and tested.

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Thomas Monjalon]

2016-06-30 15:24, Ananyev, Konstantin:
> Hi Thomas,
[...]
> 
> So are you going to apply this patch?
> Right now acl just can't be used properly in case of static library build.

Got it.
I have suggested a small rework in another mail.
Then I'll apply it promptly.

Re: [dpdk-dev] [PATCH] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

On 30/06/2016 16:28, Thomas Monjalon wrote:
> 2016-06-30 15:02, Sergio Gonzalez Monroy:
>> On 30/06/2016 13:44, Thomas Monjalon wrote:
>>> 2016-06-30 13:04, Sergio Gonzalez Monroy:
>>>> On 30/06/2016 12:38, Thomas Monjalon wrote:
>>>>> Does it need to be commented in rte.app.mk?
>>>>> The other libs are in whole-archive to support dlopen of drivers.
>>>>> But the problem here is not because of a driver use.
>>>> There seem to be a bunch of libraries under --whole-archive scope that
>>>> are not
>>>> PMDs, ie. cfgfile, cmdline...
>>>>
>>>> What is the criteria?
>>> The criteria is a bit vague. We must try to include only libs which can
>>> be used by a driver.
>>> cmdline should probably not be there.
>>> Does it make sense to use cfgfile in a driver? maybe yes.
>> So as it is, ACL autotest is broken when building static libs
>> (non-combined).
> I think the --whole-archive option must be set specifically for ACL
> with a comment explaining it is required because of weak functions:
>
> # librte_acl needs --whole-archive because of weak functions
> _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL) += --whole-archive -lrte_acl --no-whole-archive

Will do.

>> For combined libs we usually wrap libdpdk.a with --whole-archive, thus it is
>> not an issue.
>>
>> Just thinking a bit more about the 'dlopen of drivers' case you
>> mentioned before,
>> shouldn't the driver have proper dependencies and therefore need shared
>> DPDK libraries?
> It is possible to build a .so, without any DT_NEEDED entries, which will
> find the required symbols in the static linked binary.

Of course! All DPDK libraries were like that until recently.
That doesn't mean it was right though.

>> What does happen if binary/app and driver are built against different
>> library versions?
> Bad things :)
>
>> Where does it say that we do support this use case?
> It is maybe not written. But I know it is used by people wanting to load
> some PMD.so on demand while having the rest statically compiled.
> I agree it needs to be documented and probably better managed and tested.
>

Note that this only applies to apps built with DPDK build system.

In my opinion, I don't think we should be supporting such case.
But if we were to, we are probably just better of whole-archiving all 
libraries into the
application. For example, what if there was a driver wanting to use ACL 
or any
other DPDK lib not currently in the set of libs we "consider" should be 
use by drivers?

Also, from what I have seen in the list, most folks do end up using 
combined lib and
wrapping it with --whole-archive.

Sergio

[dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

Since below commit, ACL library is outside the scope of --whole-archive
and ACL autotest fails.

  RTE>>acl_autotest
  ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
  ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
  Line 1584: SSE classify with zero categories failed!
  Test Failed

This is the result of the linker picking weak over non-weak functions.

Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
---
 mk/rte.app.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 83314ca..48d5e73 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
 _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
 _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
 _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
-_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
 
 _LDLIBS-y += --whole-archive
 
+# librte_acl needs --whole-archive because of weak functions
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
 _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
 _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
-- 
2.4.11

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Thomas Monjalon]

2016-06-30 17:01, Sergio Gonzalez Monroy:
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
>  
>  _LDLIBS-y += --whole-archive
>  
> +# librte_acl needs --whole-archive because of weak functions
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost

I was suggesting to keep -lrte_acl at the same place in the group of
algorithms libraries, in order to keep an order satisfying this comment:
# Order is important: from higher level to lower level

But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.

[dpdk-dev] [PATCH v3] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

Since below commit, ACL library is outside the scope of --whole-archive
and ACL autotest fails.

  RTE>>acl_autotest
  ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
  ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
  Line 1584: SSE classify with zero categories failed!
  Test Failed

This is the result of the linker picking weak over non-weak functions.

Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v3:
- patch version history
- carry Acked-by

v2:
- add comment in makefile

 mk/rte.app.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 83314ca..48d5e73 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
 _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
 _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
 _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
-_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
 
 _LDLIBS-y += --whole-archive
 
+# librte_acl needs --whole-archive because of weak functions
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
 _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
 _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
-- 
2.4.11

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

On 30/06/2016 17:10, Thomas Monjalon wrote:
> 2016-06-30 17:01, Sergio Gonzalez Monroy:
>> --- a/mk/rte.app.mk
>> +++ b/mk/rte.app.mk
>> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
>>   
>>   _LDLIBS-y += --whole-archive
>>   
>> +# librte_acl needs --whole-archive because of weak functions
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
> I was suggesting to keep -lrte_acl at the same place in the group of
> algorithms libraries, in order to keep an order satisfying this comment:
> # Order is important: from higher level to lower level
>
> But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.
>

Sorry, I missed that.

Why is important being before jobstats and power?

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Thomas Monjalon]

2016-06-30 17:14, Sergio Gonzalez Monroy:
> On 30/06/2016 17:10, Thomas Monjalon wrote:
> > 2016-06-30 17:01, Sergio Gonzalez Monroy:
> >> --- a/mk/rte.app.mk
> >> +++ b/mk/rte.app.mk
> >> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
> >> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
> >>   
> >>   _LDLIBS-y += --whole-archive
> >>   
> >> +# librte_acl needs --whole-archive because of weak functions
> >> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
> >>   _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
> > I was suggesting to keep -lrte_acl at the same place in the group of
> > algorithms libraries, in order to keep an order satisfying this comment:
> > # Order is important: from higher level to lower level
> >
> > But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.
> >
> 
> Sorry, I missed that.
> 
> Why is important being before jobstats and power?

It is not.
But I think we need to have some groups.
And ACL is probably at the same layer level as lpm, sched, etc.

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

On 30/06/2016 17:22, Thomas Monjalon wrote:
> 2016-06-30 17:14, Sergio Gonzalez Monroy:
>> On 30/06/2016 17:10, Thomas Monjalon wrote:
>>> 2016-06-30 17:01, Sergio Gonzalez Monroy:
>>>> --- a/mk/rte.app.mk
>>>> +++ b/mk/rte.app.mk
>>>> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
>>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
>>>>    
>>>>    _LDLIBS-y += --whole-archive
>>>>    
>>>> +# librte_acl needs --whole-archive because of weak functions
>>>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
>>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
>>> I was suggesting to keep -lrte_acl at the same place in the group of
>>> algorithms libraries, in order to keep an order satisfying this comment:
>>> # Order is important: from higher level to lower level
>>>
>>> But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.
>>>
>> Sorry, I missed that.
>>
>> Why is important being before jobstats and power?
> It is not.
> But I think we need to have some groups.
> And ACL is probably at the same layer level as lpm, sched, etc.

I guess I just don't see the groups you are mentioning :)

How are timer, hash and vhost in the same group?
Wouldn't hash be in the same group as acl and lpm?

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Thomas Monjalon]

2016-07-01 09:05, Sergio Gonzalez Monroy:
> On 30/06/2016 17:22, Thomas Monjalon wrote:
> > 2016-06-30 17:14, Sergio Gonzalez Monroy:
> >> On 30/06/2016 17:10, Thomas Monjalon wrote:
> >>> 2016-06-30 17:01, Sergio Gonzalez Monroy:
> >>>> --- a/mk/rte.app.mk
> >>>> +++ b/mk/rte.app.mk
> >>>> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
> >>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
> >>>>    
> >>>>    _LDLIBS-y += --whole-archive
> >>>>    
> >>>> +# librte_acl needs --whole-archive because of weak functions
> >>>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
> >>>>    _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
> >>> I was suggesting to keep -lrte_acl at the same place in the group of
> >>> algorithms libraries, in order to keep an order satisfying this comment:
> >>> # Order is important: from higher level to lower level
> >>>
> >>> But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.
> >>>
> >> Sorry, I missed that.
> >>
> >> Why is important being before jobstats and power?
> > It is not.
> > But I think we need to have some groups.
> > And ACL is probably at the same layer level as lpm, sched, etc.
> 
> I guess I just don't see the groups you are mentioning :)

I define groups as separated by blank line :)

> How are timer, hash and vhost in the same group?

It is far from perfect and subject to improvements :)

> Wouldn't hash be in the same group as acl and lpm?

It makes sense to use hash in drivers (example: enic).

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

On 01/07/2016 11:05, Thomas Monjalon wrote:
> 2016-07-01 09:05, Sergio Gonzalez Monroy:
>> On 30/06/2016 17:22, Thomas Monjalon wrote:
>>> 2016-06-30 17:14, Sergio Gonzalez Monroy:
>>>> On 30/06/2016 17:10, Thomas Monjalon wrote:
>>>>> 2016-06-30 17:01, Sergio Gonzalez Monroy:
>>>>>> --- a/mk/rte.app.mk
>>>>>> +++ b/mk/rte.app.mk
>>>>>> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
>>>>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
>>>>>>     
>>>>>>     _LDLIBS-y += --whole-archive
>>>>>>     
>>>>>> +# librte_acl needs --whole-archive because of weak functions
>>>>>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
>>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
>>>>> I was suggesting to keep -lrte_acl at the same place in the group of
>>>>> algorithms libraries, in order to keep an order satisfying this comment:
>>>>> # Order is important: from higher level to lower level
>>>>>
>>>>> But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.
>>>>>
>>>> Sorry, I missed that.
>>>>
>>>> Why is important being before jobstats and power?
>>> It is not.
>>> But I think we need to have some groups.
>>> And ACL is probably at the same layer level as lpm, sched, etc.
>> I guess I just don't see the groups you are mentioning :)
> I define groups as separated by blank line :)
>
>> How are timer, hash and vhost in the same group?
> It is far from perfect and subject to improvements :)
>
>> Wouldn't hash be in the same group as acl and lpm?
> It makes sense to use hash in drivers (example: enic).

You have not convinced me, but I'm not going to argue more over this.

You would just prefer to do the following, right?

+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --whole-archive
_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --no-whole-archive

Re: [dpdk-dev] [PATCH v2] mk: fix acl library static linking

[Thomas Monjalon]

2016-07-01 11:27, Sergio Gonzalez Monroy:
> On 01/07/2016 11:05, Thomas Monjalon wrote:
> > 2016-07-01 09:05, Sergio Gonzalez Monroy:
> >> On 30/06/2016 17:22, Thomas Monjalon wrote:
> >>> 2016-06-30 17:14, Sergio Gonzalez Monroy:
> >>>> On 30/06/2016 17:10, Thomas Monjalon wrote:
> >>>>> 2016-06-30 17:01, Sergio Gonzalez Monroy:
> >>>>>> --- a/mk/rte.app.mk
> >>>>>> +++ b/mk/rte.app.mk
> >>>>>> @@ -76,12 +76,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
> >>>>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
> >>>>>>     
> >>>>>>     _LDLIBS-y += --whole-archive
> >>>>>>     
> >>>>>> +# librte_acl needs --whole-archive because of weak functions
> >>>>>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
> >>>>>>     _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
> >>>>> I was suggesting to keep -lrte_acl at the same place in the group of
> >>>>> algorithms libraries, in order to keep an order satisfying this comment:
> >>>>> # Order is important: from higher level to lower level
> >>>>>
> >>>>> But I have not tested if --whole-archive -lrte_acl --no-whole-archive works.
> >>>>>
> >>>> Sorry, I missed that.
> >>>>
> >>>> Why is important being before jobstats and power?
> >>> It is not.
> >>> But I think we need to have some groups.
> >>> And ACL is probably at the same layer level as lpm, sched, etc.
> >> I guess I just don't see the groups you are mentioning :)
> > I define groups as separated by blank line :)
> >
> >> How are timer, hash and vhost in the same group?
> > It is far from perfect and subject to improvements :)
> >
> >> Wouldn't hash be in the same group as acl and lpm?
> > It makes sense to use hash in drivers (example: enic).
> 
> You have not convinced me, but I'm not going to argue more over this.

But you have convinced me (I was already convinced) that more cleanups
and explanations are needed in this area :)

> You would just prefer to do the following, right?
> 
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --whole-archive
> _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --no-whole-archive

Yes for this fix.
Later we can improve few things, thanks.

[dpdk-dev] [PATCH v4 1/2] mk: allow duplicate linker flags in libraries list

[Sergio Gonzalez Monroy]

Since [1] duplicates in LDLIBS are removed. The side effect is that it
does not distinguish between libraries or linker flags.

This patch allows multiple linker flags in LDLIBS, such as
--whole-archive.

[1] Commit: edf4d331dcdb ("mk: eliminate duplicates from libraries list")

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
---
 mk/rte.app.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 83314ca..bf8bcf9 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -166,7 +166,8 @@ LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
 
 # Eliminate duplicates without sorting
 LDLIBS := $(shell echo $(LDLIBS) | \
-	awk '{for (i = 1; i <= NF; i++) { if (!seen[$$i]++) print $$i }}')
+	awk '{for (i = 1; i <= NF; i++) { \
+		if ($$i !~ /^-l.*/ || !seen[$$i]++) print $$i }}')
 
 ifeq ($(RTE_DEVEL_BUILD)$(CONFIG_RTE_BUILD_SHARED_LIB),yy)
 LDFLAGS += -rpath=$(RTE_SDK_BIN)/lib
-- 
2.4.11

[dpdk-dev] [PATCH v4 2/2] mk: fix acl library static linking

[Sergio Gonzalez Monroy]

Since below commit, ACL library is outside the scope of --whole-archive
and ACL autotest fails.

  RTE>>acl_autotest
  ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
  ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
  Line 1584: SSE classify with zero categories failed!
  Test Failed

This is the result of the linker picking weak over non-weak functions.

Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
---
v4:
- keep order of acl in _LDLIBS and wrap it with --whole-archive

v3:
- patch version history
- carry Acked-by

v2:
- add comment in makefile

 mk/rte.app.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index bf8bcf9..ea22961 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -76,7 +76,10 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
 _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
 _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
 _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
+# librte_acl needs --whole-archive because of weak functions
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --whole-archive
 _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --no-whole-archive
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
 
-- 
2.4.11

Re: [dpdk-dev] [PATCH v4 2/2] mk: fix acl library static linking

[Thomas Monjalon]

2016-07-01 15:38, Sergio Gonzalez Monroy:
> Since below commit, ACL library is outside the scope of --whole-archive
> and ACL autotest fails.
> 
>   RTE>>acl_autotest
>   ACL: allocation of 25166728 bytes on socket 9 for ACL_acl_ctx failed
>   ACL: rte_acl_add_rules(acl_ctx): rule #1 is invalid
>   Line 1584: SSE classify with zero categories failed!
>   Test Failed
> 
> This is the result of the linker picking weak over non-weak functions.
> 
> Fixes: 95dc3c3cf31c ("mk: reduce scope of whole-archive static linking")
> 
> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

Series applied, thanks