telemetry: fix shared build for make

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

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.
  

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
  

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>
  

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