Message ID | 20200127154402.4008069-1-thomas@monjalon.net (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id CE2BFA0531; Mon, 27 Jan 2020 16:44:17 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 028911BF98; Mon, 27 Jan 2020 16:44:17 +0100 (CET) Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) by dpdk.org (Postfix) with ESMTP id 16C931BF87 for <dev@dpdk.org>; Mon, 27 Jan 2020 16:44:16 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 2B53A4B4; Mon, 27 Jan 2020 10:44:14 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Mon, 27 Jan 2020 10:44:14 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; s=mesmtp; bh=n1uzv9e47c 7GztSf2bQwhotENUSBjDG/cJOtaR9h5m0=; b=C7DtcDaJ2dq51UbhWmxpxDDFiD rksFnIBV5G9PglZGMcvh7NMo+CLY02VDXOHAaSuwQ2ILchcXyJqsEEkI3RBKsweI gUNU71mfBZYWBKk43NkZ50rdmAPLpt2/br0T9C3Zb9b8Yxq0g3ayfgHmOK40SPp9 LbHWWJu4F6mN7dj4U= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:date:from :in-reply-to:message-id:mime-version:references:subject:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; bh=n1uzv9e47c7GztSf2bQwhotENUSBjDG/cJOtaR9h5m0=; b=mAymu83v NMTsus9KeHmNpto5ybwgm3w5Ayf9TnZW/WgNb2BKu4W0YOaReRRJuI4X3sP7IpnE aaX91NeUrxQ76rqENsSUJEfXKVCrrULVDdkQjDXkkUlgL7bRcBGdSG2SP4vbAwrl iKDFR7Dz6pEhHENmnSvtOMcOLBQ8yU+ezvwWS22UIT15ucan6+V7dNpP5zH5bVJx nlRAwfbzniNbuKmwZCjmKoIuT4MoXbNgL4Z0oXJnqwn20NfFN8UOeNgoQI3zW4sV 6dinZI1FX9fmzKYtW9Y30ls0J07qm5kFdQom1ZN0dAPeS/+d5qoLmZ05QKEHAKGG J+kZhhB0Iu3L5w== X-ME-Sender: <xms:TQUvXv1_IuakLfN1OmpKPTtD0MVATh8GDwthERu9fSiUoFnGRX0kpA> X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedrfedvgdejkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecunecujfgurhephffvufffkffojghfggfgsedtkeertd ertddtnecuhfhrohhmpefvhhhomhgrshcuofhonhhjrghlohhnuceothhhohhmrghssehm ohhnjhgrlhhonhdrnhgvtheqnecukfhppeejjedrudefgedrvddtfedrudekgeenucevlh hushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshes mhhonhhjrghlohhnrdhnvght X-ME-Proxy: <xmx:TQUvXp02bHn4ywyBVIenybaOiJAJfhkX_vQ8ozu16GwwHomkeNI0aA> <xmx:TQUvXofODmuKf7ZNhkuPIb-8v4hM8-wdARBjhUfsxLKpDkNbo1Jo-Q> <xmx:TQUvXl9v3VWqkcUdwmUM8n2bMe_-X9aZwllw5eW1nGJ0RT80d_9M5w> <xmx:TQUvXpw63gjes6M0Q9OsMRO9JUYm0darKuClVFxAFM3lvtMXhRFLtQ> Received: from xps.monjalon.net (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id AD9CD3280059; Mon, 27 Jan 2020 10:44:12 -0500 (EST) From: Thomas Monjalon <thomas@monjalon.net> To: dev@dpdk.org Cc: bruce.richardson@intel.com Date: Mon, 27 Jan 2020 16:43:58 +0100 Message-Id: <20200127154402.4008069-1-thomas@monjalon.net> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200116071656.1663967-1-thomas@monjalon.net> References: <20200116071656.1663967-1-thomas@monjalon.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [PATCH v2 0/4] add static ibverbs in meson X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Series | add static ibverbs in meson | |
Message
Thomas Monjalon
Jan. 27, 2020, 3:43 p.m. UTC
This is the follow-up of the feature I added one year ago: static linkage of libibverbs in mlx PMDs. The first implementation was focused on "make". This second round does the same with "meson". changes in v2: - split mlx patch for normal addition and workarounds - fix ldflags for ibverbs installed in a standard directory - fix libs order leading to undefined references - add doc for hidden_deps - improve explanations in commit logs Thomas Monjalon (4): net/mlx: add static ibverbs linkage with meson buildtools: get static mlx dependencies for meson build: allow hiding dependencies from pkg-config net/mlx: workaround static linkage with meson buildtools/meson.build | 2 ++ buildtools/options-ibverbs-static.sh | 10 ++++++++-- doc/guides/contributing/coding_style.rst | 6 ++++++ doc/guides/nics/mlx4.rst | 4 ++++ doc/guides/nics/mlx5.rst | 4 ++++ drivers/meson.build | 9 ++++++--- drivers/net/mlx4/meson.build | 19 +++++++++++++++---- drivers/net/mlx5/meson.build | 19 +++++++++++++++---- meson_options.txt | 4 ++-- 9 files changed, 62 insertions(+), 15 deletions(-)
Comments
On Mon, Jan 27, 2020 at 04:43:58PM +0100, Thomas Monjalon wrote: > This is the follow-up of the feature I added one year ago: > static linkage of libibverbs in mlx PMDs. > The first implementation was focused on "make". > This second round does the same with "meson". > > > changes in v2: > - split mlx patch for normal addition and workarounds > - fix ldflags for ibverbs installed in a standard directory > - fix libs order leading to undefined references > - add doc for hidden_deps > - improve explanations in commit logs > > > Thomas Monjalon (4): > net/mlx: add static ibverbs linkage with meson > buildtools: get static mlx dependencies for meson > build: allow hiding dependencies from pkg-config > net/mlx: workaround static linkage with meson > > Hi Thomas, as we discussed offline, I really don't like the necessity of the hidden_deps part of this, so I've tried coming up with some other solutions. For example, in my testing I get the same result with the following diff instead of the second two patches (just showing for mlx5 - changes for mlx4 are identical): --- a/drivers/net/mlx5/meson.build +++ b/drivers/net/mlx5/meson.build @@ -31,10 +31,7 @@ foreach libname:libnames endif if lib.found() libs += lib - if static_ibverbs - # Build without adding shared libs to Requires.private - hidden_deps += lib.partial_dependency(compile_args:true) - else + if not static_ibverbs ext_deps += lib endif else @@ -44,8 +41,10 @@ foreach libname:libnames endforeach if build and static_ibverbs # Add static deps ldflags to internal apps and Libs.private - ldflags = run_command(ldflags_ibverbs_static, check:true).stdout() - ext_deps += declare_dependency(link_args:ldflags.split()) + ibv_ldflags = run_command(ldflags_ibverbs_static, check:true).stdout() + ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout() + ext_deps += declare_dependency(compile_args: ibv_cflags.split(), + link_args:ibv_ldflags.split()) endif if build By doing things this way - assuming it works in your tests too - we avoid any need to change anything in the common drivers code. How I tested this was by: * doing a local rdma-core build as described in the docs and exporting PKG_CONFIG_PATH to point to the .pc file * doing builds with all 4 of your patches applied (one static linking of apps, one shared, just in case) * doing builds with the first two patches and the above diff. * compare the meson.build files and libdpdk.pc files for the two cases. In my tests the second case as additional -I flags, but otherwise identical. Regards, /Bruce
04/02/2020 12:48, Bruce Richardson: > as we discussed offline, I really don't like the necessity of the > hidden_deps part of this, so I've tried coming up with some other > solutions. Thanks for looking closely at these patches. > For example, in my testing I get the same result with the > following diff instead of the second two patches (just showing for mlx5 - > changes for mlx4 are identical): [...] > - # Build without adding shared libs to Requires.private > - hidden_deps += lib.partial_dependency(compile_args:true) [...] > + ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout() [...] > By doing things this way - assuming it works in your tests too - we avoid > any need to change anything in the common drivers code. Yes, you hide the dependency by getting cflags directly with pkg-config. I wanted to avoid such solution because I was trying to use as much as possible the meson infrastructure. I think your solution relying on one more call to pkg-config is acceptable. I will test it and will review whether we get all corner cases. In the meantime I discovered we are overlinking with meson when using the dlopen linking option. I will try to fix it as well with the same method. Thanks
On Tue, Feb 04, 2020 at 03:27:50PM +0100, Thomas Monjalon wrote: > 04/02/2020 12:48, Bruce Richardson: > > as we discussed offline, I really don't like the necessity of the > > hidden_deps part of this, so I've tried coming up with some other > > solutions. > > Thanks for looking closely at these patches. > > > For example, in my testing I get the same result with the > > following diff instead of the second two patches (just showing for mlx5 - > > changes for mlx4 are identical): > [...] > > - # Build without adding shared libs to Requires.private > > - hidden_deps += lib.partial_dependency(compile_args:true) > [...] > > + ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout() > [...] > > By doing things this way - assuming it works in your tests too - we avoid > > any need to change anything in the common drivers code. > > Yes, you hide the dependency by getting cflags directly with pkg-config. > I wanted to avoid such solution because I was trying to use as much > as possible the meson infrastructure. > > I think your solution relying on one more call to pkg-config is acceptable. > I will test it and will review whether we get all corner cases. > Thanks. It's not really ideal, but this is likely always going to be a bit flakey since we can't use distro-supplied .a files, and your scripting is needed to prevent even accidentally taking a non-custom-build rdma-core file. Furthermore, I see that while meson tracks PKG_CONFIG_PATH value itself for finding things, this does not get tracked between runs for shell calls. This can catch one out, for example: PKG_CONFIG_PATH=/path/to/pc/files meson build will work correctly for everything. However, if one does a reconfigure subsequently doing e.g. ninja reconfigure, meson will correctly pick up the .pc files, but the ibverbs-static script, or any run_command calls to pkg-config won't as it's not actually in the environment :-( > In the meantime I discovered we are overlinking with meson > when using the dlopen linking option. > I will try to fix it as well with the same method. > Right. Overlinking is probably less serious an issue though. Does it cause any real-world problems? /Bruce
04/02/2020 15:33, Bruce Richardson: > On Tue, Feb 04, 2020 at 03:27:50PM +0100, Thomas Monjalon wrote: > > 04/02/2020 12:48, Bruce Richardson: > > > as we discussed offline, I really don't like the necessity of the > > > hidden_deps part of this, so I've tried coming up with some other > > > solutions. > > > > Thanks for looking closely at these patches. > > > > > For example, in my testing I get the same result with the > > > following diff instead of the second two patches (just showing for mlx5 - > > > changes for mlx4 are identical): > > [...] > > > - # Build without adding shared libs to Requires.private > > > - hidden_deps += lib.partial_dependency(compile_args:true) > > [...] > > > + ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout() > > [...] > > > By doing things this way - assuming it works in your tests too - we avoid > > > any need to change anything in the common drivers code. > > > > Yes, you hide the dependency by getting cflags directly with pkg-config. > > I wanted to avoid such solution because I was trying to use as much > > as possible the meson infrastructure. > > > > I think your solution relying on one more call to pkg-config is acceptable. > > I will test it and will review whether we get all corner cases. > > > > Thanks. > It's not really ideal, but this is likely always going to be a bit flakey > since we can't use distro-supplied .a files, and your scripting is needed > to prevent even accidentally taking a non-custom-build rdma-core file. > Furthermore, I see that while meson tracks PKG_CONFIG_PATH value itself for > finding things, this does not get tracked between runs for shell calls. > This can catch one out, for example: > > PKG_CONFIG_PATH=/path/to/pc/files meson build > > will work correctly for everything. However, if one does a reconfigure > subsequently doing e.g. ninja reconfigure, meson will correctly pick up the > .pc files, but the ibverbs-static script, or any run_command calls to > pkg-config won't as it's not actually in the environment :-( In my setup, I export PKG_CONFIG_PATH, so it is not an issue. But I understand your point that we may hit the issue. That's why I will work in meson upstream to avoid all of this in future. > > In the meantime I discovered we are overlinking with meson > > when using the dlopen linking option. > > I will try to fix it as well with the same method. > > > Right. Overlinking is probably less serious an issue though. Does it cause > any real-world problems? Overlinking defeats the benefit of dlopen. The idea of dlopen is to avoid having ibverbs as mandatory DPDK dependency. The ibverbs lib is loaded with dlopen only if probing a Mellanox device. The dlopen solution makes the PMD really like an optional plugin, same as for static linkage in the PMD.
On Tue, Feb 04, 2020 at 04:14:46PM +0100, Thomas Monjalon wrote: > 04/02/2020 15:33, Bruce Richardson: > > On Tue, Feb 04, 2020 at 03:27:50PM +0100, Thomas Monjalon wrote: > > > 04/02/2020 12:48, Bruce Richardson: > > > > as we discussed offline, I really don't like the necessity of the > > > > hidden_deps part of this, so I've tried coming up with some other > > > > solutions. > > > > > > Thanks for looking closely at these patches. > > > > > > > For example, in my testing I get the same result with the > > > > following diff instead of the second two patches (just showing for mlx5 - > > > > changes for mlx4 are identical): > > > [...] > > > > - # Build without adding shared libs to Requires.private > > > > - hidden_deps += lib.partial_dependency(compile_args:true) > > > [...] > > > > + ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout() > > > [...] > > > > By doing things this way - assuming it works in your tests too - we avoid > > > > any need to change anything in the common drivers code. > > > > > > Yes, you hide the dependency by getting cflags directly with pkg-config. > > > I wanted to avoid such solution because I was trying to use as much > > > as possible the meson infrastructure. > > > > > > I think your solution relying on one more call to pkg-config is acceptable. > > > I will test it and will review whether we get all corner cases. > > > > > > > Thanks. > > It's not really ideal, but this is likely always going to be a bit flakey > > since we can't use distro-supplied .a files, and your scripting is needed > > to prevent even accidentally taking a non-custom-build rdma-core file. > > Furthermore, I see that while meson tracks PKG_CONFIG_PATH value itself for > > finding things, this does not get tracked between runs for shell calls. > > This can catch one out, for example: > > > > PKG_CONFIG_PATH=/path/to/pc/files meson build > > > > will work correctly for everything. However, if one does a reconfigure > > subsequently doing e.g. ninja reconfigure, meson will correctly pick up the > > .pc files, but the ibverbs-static script, or any run_command calls to > > pkg-config won't as it's not actually in the environment :-( > > In my setup, I export PKG_CONFIG_PATH, so it is not an issue. > But I understand your point that we may hit the issue. > That's why I will work in meson upstream to avoid all of this in future. > +1 > > > > In the meantime I discovered we are overlinking with meson > > > when using the dlopen linking option. > > > I will try to fix it as well with the same method. > > > > > Right. Overlinking is probably less serious an issue though. Does it cause > > any real-world problems? > > Overlinking defeats the benefit of dlopen. > The idea of dlopen is to avoid having ibverbs as mandatory DPDK dependency. > The ibverbs lib is loaded with dlopen only if probing a Mellanox device. > The dlopen solution makes the PMD really like an optional plugin, > same as for static linkage in the PMD. > Right, I understand the issue now.