From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 076BB239 for ; Thu, 8 Nov 2018 12:59:28 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Nov 2018 03:59:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,478,1534834800"; d="scan'208";a="248025775" Received: from klaatz-mobl.ger.corp.intel.com (HELO [10.237.220.111]) ([10.237.220.111]) by orsmga004.jf.intel.com with ESMTP; 08 Nov 2018 03:59:25 -0800 To: Thomas Monjalon , harry.van.haaren@intel.com Cc: dev@dpdk.org, gaetan.rivet@6wind.com, mattias.ronnblom@ericsson.com, bruce.richardson@intel.com, radu.nicolau@intel.com, ciara.power@intel.com, brian.archbold@intel.com References: <20181107181018.80224-1-kevin.laatz@intel.com> <6475218.XGFdfmr2Ql@xps> From: "Laatz, Kevin" Message-ID: <8747d99c-85c1-4301-b0a2-998dec934d8b@intel.com> Date: Thu, 8 Nov 2018 11:59:24 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <6475218.XGFdfmr2Ql@xps> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH] telemetry: fix shared build for make X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Nov 2018 11:59:29 -0000 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 >> Signed-off-by: Kevin Laatz > [...] >> --- 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