telemetry: fix shared build for make
Checks
Commit Message
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(+)
Comments
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
@@ -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