DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] telemetry: fix shared build for make
@ 2018-11-07 18:10 Kevin Laatz
  2018-11-07 21:08 ` Thomas Monjalon
  2018-11-09 22:19 ` Ferruh Yigit
  0 siblings, 2 replies; 5+ messages in thread
From: Kevin Laatz @ 2018-11-07 18:10 UTC (permalink / raw)
  To: dev
  Cc: harry.van.haaren, gaetan.rivet, thomas, mattias.ronnblom,
	bruce.richardson, radu.nicolau, ciara.power, brian.archbold,
	Kevin Laatz

Currently, telemetry is not working for shared builds in make.

The --as-needed flag is preventing telemetry from being linked as there are
no direct API calls from the app to telemetry. This is causing the
--telemetry option to not be recognized by EAL.
Telemetry registers it's EAL option using the RTE_INIT constructor. Since
EAL's option parsing is done before the plugins init, the --telemetry
option isn't registered at the time of parsing, and as a result, the
--telemetry option is not being recognized.

This patch fixes this issue by explicitly linking telemetry to the
application by setting the "--no-as-needed" flag for the library in
mk/rte.app.mk.

Fixes: 8877ac688b52 ("telemetry: introduce infrastructure")

Reported-by: Yanjie Xu <yanjie.xu@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 mk/rte.app.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 3ebc4e64c..5699d979d 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -50,9 +50,11 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
 _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_TELEMETRY)      += --no-as-needed
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --whole-archive
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += -lrte_telemetry -ljansson
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --no-whole-archive
+_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --as-needed
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_METRICS)        += -lrte_metrics
 _LDLIBS-$(CONFIG_RTE_LIBRTE_BITRATE)        += -lrte_bitratestats
-- 
2.17.2

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] telemetry: fix shared build for make
  2018-11-07 18:10 [dpdk-dev] [PATCH] telemetry: fix shared build for make Kevin Laatz
@ 2018-11-07 21:08 ` Thomas Monjalon
  2018-11-08 11:59   ` Laatz, Kevin
  2018-11-09 22:19 ` Ferruh Yigit
  1 sibling, 1 reply; 5+ messages in thread
From: Thomas Monjalon @ 2018-11-07 21:08 UTC (permalink / raw)
  To: Kevin Laatz, harry.van.haaren
  Cc: dev, gaetan.rivet, mattias.ronnblom, bruce.richardson,
	radu.nicolau, ciara.power, brian.archbold

07/11/2018 19:10, Kevin Laatz:
> Currently, telemetry is not working for shared builds in make.
> 
> The --as-needed flag is preventing telemetry from being linked as there are
> no direct API calls from the app to telemetry. This is causing the
> --telemetry option to not be recognized by EAL.
> Telemetry registers it's EAL option using the RTE_INIT constructor. Since
> EAL's option parsing is done before the plugins init, the --telemetry
> option isn't registered at the time of parsing, and as a result, the
> --telemetry option is not being recognized.

I do not understand how linking is related to parsing done before plugin init.
Do you mean telemetry cannot be linked with dlopen?

It makes me think that we could have avoided all the rte_option
infrastructure by using plugin loading.
If it is built statically, we can enable telemetry with a compilation
option. If it is built as a shared library, the -d option is enough.

About the patch itself,

> This patch fixes this issue by explicitly linking telemetry to the
> application by setting the "--no-as-needed" flag for the library in
> mk/rte.app.mk.
> 
> Fixes: 8877ac688b52 ("telemetry: introduce infrastructure")
> 
> Reported-by: Yanjie Xu <yanjie.xu@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
[...]
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --no-as-needed
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --whole-archive
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += -lrte_telemetry -ljansson
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --no-whole-archive
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --as-needed

It is forcing --as-needed even if it was disabled in LDFLAGS.
I think this form may be better:
	--push-state,--as-needed
	--pop-state
but it may not be supported by all linkers.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] telemetry: fix shared build for make
  2018-11-07 21:08 ` Thomas Monjalon
@ 2018-11-08 11:59   ` Laatz, Kevin
  0 siblings, 0 replies; 5+ messages in thread
From: Laatz, Kevin @ 2018-11-08 11:59 UTC (permalink / raw)
  To: Thomas Monjalon, harry.van.haaren
  Cc: dev, gaetan.rivet, mattias.ronnblom, bruce.richardson,
	radu.nicolau, ciara.power, brian.archbold

On 07/11/2018 21:08, Thomas Monjalon wrote:
> 07/11/2018 19:10, Kevin Laatz:
>> Currently, telemetry is not working for shared builds in make.
>>
>> The --as-needed flag is preventing telemetry from being linked as there are
>> no direct API calls from the app to telemetry. This is causing the
>> --telemetry option to not be recognized by EAL.
>> Telemetry registers it's EAL option using the RTE_INIT constructor. Since
>> EAL's option parsing is done before the plugins init, the --telemetry
>> option isn't registered at the time of parsing, and as a result, the
>> --telemetry option is not being recognized.
> I do not understand how linking is related to parsing done before plugin init.
> Do you mean telemetry cannot be linked with dlopen?
>
> It makes me think that we could have avoided all the rte_option
> infrastructure by using plugin loading.
> If it is built statically, we can enable telemetry with a compilation
> option. If it is built as a shared library, the -d option is enough.
In EAL, option parsing happens before plugin init. This ordering is 
required in EAL since
plugins have EAL options that need to be parsed.

Telemetry registers it's EAL option using RTE_INIT constructor, so we 
cannot use telemetry
as a plugin since the option won't exist yet at the time of parsing due 
to the ordering of the
parsing and the init (where the telemetry option will be registered).
The -d option is also not enough as the telemetry option won't exist 
until plugin init is
done, which is too late.

Linking the library to the application solves this.

>
> About the patch itself,
>
>> This patch fixes this issue by explicitly linking telemetry to the
>> application by setting the "--no-as-needed" flag for the library in
>> mk/rte.app.mk.
>>
>> Fixes: 8877ac688b52 ("telemetry: introduce infrastructure")
>>
>> Reported-by: Yanjie Xu <yanjie.xu@intel.com>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> [...]
>> --- a/mk/rte.app.mk
>> +++ b/mk/rte.app.mk
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --no-as-needed
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --whole-archive
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += -lrte_telemetry -ljansson
>>   _LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --no-whole-archive
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_TELEMETRY)      += --as-needed
> It is forcing --as-needed even if it was disabled in LDFLAGS.
> I think this form may be better:
> 	--push-state,--as-needed
> 	--pop-state
> but it may not be supported by all linkers.
>
This does seem like a cleaner way to do this, and we could also use it 
for the
--whole-archive flag. However, if it is not supported by all linkers 
then I don't think
we should use it.

Regards,
Kevin

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] telemetry: fix shared build for make
  2018-11-07 18:10 [dpdk-dev] [PATCH] telemetry: fix shared build for make Kevin Laatz
  2018-11-07 21:08 ` Thomas Monjalon
@ 2018-11-09 22:19 ` Ferruh Yigit
  2018-11-12  0:23   ` Thomas Monjalon
  1 sibling, 1 reply; 5+ messages in thread
From: Ferruh Yigit @ 2018-11-09 22:19 UTC (permalink / raw)
  To: Kevin Laatz, dev
  Cc: harry.van.haaren, gaetan.rivet, thomas, mattias.ronnblom,
	bruce.richardson, radu.nicolau, ciara.power, brian.archbold

On 11/7/2018 6:10 PM, Kevin Laatz wrote:
> Currently, telemetry is not working for shared builds in make.
> 
> The --as-needed flag is preventing telemetry from being linked as there are
> no direct API calls from the app to telemetry. This is causing the
> --telemetry option to not be recognized by EAL.
> Telemetry registers it's EAL option using the RTE_INIT constructor. Since
> EAL's option parsing is done before the plugins init, the --telemetry
> option isn't registered at the time of parsing, and as a result, the
> --telemetry option is not being recognized.
> 
> This patch fixes this issue by explicitly linking telemetry to the
> application by setting the "--no-as-needed" flag for the library in
> mk/rte.app.mk.
> 
> Fixes: 8877ac688b52 ("telemetry: introduce infrastructure")
> 
> Reported-by: Yanjie Xu <yanjie.xu@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] telemetry: fix shared build for make
  2018-11-09 22:19 ` Ferruh Yigit
@ 2018-11-12  0:23   ` Thomas Monjalon
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Monjalon @ 2018-11-12  0:23 UTC (permalink / raw)
  To: Kevin Laatz
  Cc: dev, Ferruh Yigit, harry.van.haaren, gaetan.rivet,
	mattias.ronnblom, bruce.richardson, radu.nicolau, ciara.power,
	brian.archbold

09/11/2018 23:19, Ferruh Yigit:
> On 11/7/2018 6:10 PM, Kevin Laatz wrote:
> > Currently, telemetry is not working for shared builds in make.
> > 
> > The --as-needed flag is preventing telemetry from being linked as there are
> > no direct API calls from the app to telemetry. This is causing the
> > --telemetry option to not be recognized by EAL.
> > Telemetry registers it's EAL option using the RTE_INIT constructor. Since
> > EAL's option parsing is done before the plugins init, the --telemetry
> > option isn't registered at the time of parsing, and as a result, the
> > --telemetry option is not being recognized.
> > 
> > This patch fixes this issue by explicitly linking telemetry to the
> > application by setting the "--no-as-needed" flag for the library in
> > mk/rte.app.mk.
> > 
> > Fixes: 8877ac688b52 ("telemetry: introduce infrastructure")
> > 
> > Reported-by: Yanjie Xu <yanjie.xu@intel.com>
> > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-11-12  0:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 18:10 [dpdk-dev] [PATCH] telemetry: fix shared build for make Kevin Laatz
2018-11-07 21:08 ` Thomas Monjalon
2018-11-08 11:59   ` Laatz, Kevin
2018-11-09 22:19 ` Ferruh Yigit
2018-11-12  0:23   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).