From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f175.google.com (mail-wr0-f175.google.com [209.85.128.175]) by dpdk.org (Postfix) with ESMTP id 96C8F1B40F for ; Mon, 5 Feb 2018 16:55:09 +0100 (CET) Received: by mail-wr0-f175.google.com with SMTP id y3so20280299wrh.3 for ; Mon, 05 Feb 2018 07:55:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=16AsY9ycAeIm34ZgzgZ2V91HsSQF2snS9jOu1pdJyNk=; b=raeLtIG1H0G/4M9mTMnhMVSyZwQalCmP9oMqWr44aUcFl6QsJ9R1DoK12T2y/7RsGF gALjD6nzPH5UUUbTTOW510LygNkJxa1hzJbgAeN++ph3VvcHvaNfaCO74JPPmxi14EtH XhmUqh6Zp/CpEU9SHEu4mNEvCaLik5T34veo23lr0EakUS7oLdjku/mDmD+Z2d0S8ay8 XXaoNRS4Rq7SvZzx4e0oM/FQSbujmB7TTcDITbm0KH2oQtaudGJ3j2L4uPH9/yOP05Ku Tpiqz1zS86BVrf3KZWXGTCDtbRCizXYOjslOw/038qo6LqudLYkE2ToSpw7R80e+XZN9 SJtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=16AsY9ycAeIm34ZgzgZ2V91HsSQF2snS9jOu1pdJyNk=; b=SPLWY0RQnLZ7OkKfi9DOL2tEfljsvomAQ25CPIp+gLkTo8tPw3E+mynnGDR9Xo0itp VR1bHkS6zb8zFoWxhU1j8DVAEIVV2rHtUuKWn2Ya5QVxoAim6CWOBFDNiZcCY13dJ8n9 l4tjh3Tt/NCl5DLw/Duqz9ypZsRvJUA1Wx7goM9GFsDtYgflfnzcVkfxt9o0zYNQVTE0 ZHjSFOQZzvOHIiPJb3oNkWXv/dJeYiZ9FINYpncVc9OwQwOjPSl66NcFTUqTgUbX6xFW /yrNbHWGfTEBoZH9C3q9JK5YiqUXkPL0FG1na4jpmbnE2Zk+TJFpcqnAeGhgWCoZVhIx RXpw== X-Gm-Message-State: APf1xPDjUPtrLtOsaFP7KVle5E8hvMALz1p5I8aC1j5SsgtspzuYhMqk BzwbQyMnw+5BBKk96wvsKgemmg== X-Google-Smtp-Source: AH8x225Yhm4bW0YwOHTfb6iBWW/C6oBJzbbpHub1QPBCx4vybCu2g2vOQ7u+Q8AN4JLBU121GcQ0Wg== X-Received: by 10.223.197.12 with SMTP id q12mr9844111wrf.147.1517846109343; Mon, 05 Feb 2018 07:55:09 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id z1sm3227998wre.25.2018.02.05.07.55.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Feb 2018 07:55:08 -0800 (PST) Date: Mon, 5 Feb 2018 16:54:55 +0100 From: Adrien Mazarguil To: Marcelo Ricardo Leitner Cc: Thomas Monjalon , "Van Haaren, Harry" , "dev@dpdk.org" , Shahaf Shuler , Nelio Laranjeiro Message-ID: <20180205155455.GN4256@6wind.com> References: <20180202144736.8239-1-adrien.mazarguil@6wind.com> <20180205125806.GE27676@localhost.localdomain> <20180205134446.GG4256@6wind.com> <1992025.8JADqR79Gx@xps> <20180205143734.GG27676@localhost.localdomain> <20180205145917.GJ4256@6wind.com> <20180205152942.GH27676@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180205152942.GH27676@localhost.localdomain> Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/mlx: version rdma-core glue libraries 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: Mon, 05 Feb 2018 15:55:09 -0000 On Mon, Feb 05, 2018 at 01:29:42PM -0200, Marcelo Ricardo Leitner wrote: > On Mon, Feb 05, 2018 at 03:59:18PM +0100, Adrien Mazarguil wrote: > > On Mon, Feb 05, 2018 at 12:37:34PM -0200, Marcelo Ricardo Leitner wrote: > > > On Mon, Feb 05, 2018 at 03:16:21PM +0100, Thomas Monjalon wrote: > > > > 05/02/2018 14:44, Adrien Mazarguil: > > > > > On Mon, Feb 05, 2018 at 10:58:06AM -0200, Marcelo Ricardo Leitner wrote: > > > > > > On Mon, Feb 05, 2018 at 12:24:23PM +0000, Van Haaren, Harry wrote: > > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marcelo Ricardo Leitner > > > > > > > > Sent: Monday, February 5, 2018 12:14 PM > > > > > > > > To: Adrien Mazarguil > > > > > > > > Cc: Thomas Monjalon ; dev@dpdk.org; Shahaf Shuler > > > > > > > > ; Nelio Laranjeiro > > > > > > > > Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/mlx: version rdma-core glue > > > > > > > > libraries > > > > > > > > > > > > > > > > On Mon, Feb 05, 2018 at 12:24:02PM +0100, Adrien Mazarguil wrote: > > > > > > > > > On Sun, Feb 04, 2018 at 03:29:38PM +0100, Thomas Monjalon wrote: > > > > > > > > > > 02/02/2018 17:46, Adrien Mazarguil: > > > > > > > > > > > --- a/drivers/net/mlx4/Makefile > > > > > > > > > > > +++ b/drivers/net/mlx4/Makefile > > > > > > > > > > > @@ -33,7 +33,9 @@ include $(RTE_SDK)/mk/rte.vars.mk > > > > > > > > > > > > > > > > > > > > > > # Library name. > > > > > > > > > > > LIB = librte_pmd_mlx4.a > > > > > > > > > > > -LIB_GLUE = librte_pmd_mlx4_glue.so > > > > > > > > > > > +LIB_GLUE = $(LIB_GLUE_BASE).$(LIB_GLUE_VERSION) > > > > > > > > > > > +LIB_GLUE_BASE = librte_pmd_mlx4_glue.so > > > > > > > > > > > +LIB_GLUE_VERSION = 18.02.1 > > > > > > > > > > > > > > > > > > > > You should use the version number of the release, i.e. 18.02.0 > > > > > > > > > > Ideally, you should retrieve it from rte_version.h. > > > > > > > > > > > > > > > > > > Keep in mind this only needs to be updated when the glue API gets > > > > > > > > modified, > > > > > > > > > and this "18.02.1" string may remain unmodified for subsequent DPDK > > > > > > > > > releases, probably as long as the PMD doesn't use any new rdma-core calls. > > > > > > > > > > > > > > > > > > We've already backported this patch to 17.02 and 17.11, both requiring > > > > > > > > > different sets of Verbs calls and thus a different version, hence the > > > > > > > > added > > > > > > > > > "18.02" as a starting point. The last digit may have to be modified > > > > > > > > possibly > > > > > > > > > several times between official DPDK releases while work is being done on > > > > > > > > the > > > > > > > > > PMD (i.e. per commit). > > > > > > > > > > > > > > > > > > In short it's not meant to follow DPDK's public versioning scheme. If you > > > > > > > > > really think it should, doing so will make things more complex in the > > > > > > > > > Makefile, which will have to parse rte_version.h. What's your opinion? > > > > > > > > > > > > > > > > What about appending date +%s output to it? It would be stricter and > > > > > > > > automated. > > > > > > > > > > > > > > Adding current timestamp or date into a build breaks reproducibility of builds, so is > > > > > > > generally not recommended. > > > > > > > > > > > > Then the sha1sum of mlx4_glue.h. > > > > > > With this the size check I mentioned on the other patch would become > > > > > > redundant and unnecessary. > > > > > > > > > > Using a strong hash algorithm to version a library/symbol, while possible, > > > > > seems a bit overkill and results in ugliness: > > > > > > > > > > librte_pmd_mlx4.so.c4ca4eaf2fe975ead83453458f4f56db49e724f3 > > > > > > Ugh yes, but it wouldn't need to be that visible. A pointer on > > > mlx*_glue and a define on PMD would be enough already. As in, an > > > extended check to the versioning. > > > > I thought you suggested this as a replacement. I'm not sure we need or want > > to go this far. The current string comparison is really not worse than > > standard symbol versioning, which doesn't check symbol properties besides > > whether they are functions or other objects. We could have used C++ with > > automatically mangled symbol names for that, however that again would make > > things way more complex than necessary. > > > > > > > Using a weak one like CRC32 for a shorter name poses a risk of > > > > > collision. Moreover the next time someone decides to update all version > > > > > notices or modify a comment will impact that hash. We'd need to isolate the > > > > > symbol definition itself, ignore parameter names in function prototypes and > > > > > only then we may get a somewhat meaningful hash describing a given ABI. > > > > > > That's what I meant with stricter. Yes it would catch such > > > situations, but you tell me on how much we want to protect/restrict > > > here. Do you see a reason for building only the dpdk/pmd side and not > > > the glue library at a time? > > > > No, they're always built together. We're only adding this versioning to > > avoid issues when users somehow end up with several DPDK versions installed > > on their system, or with leftovers of previous releases lying around. That's > > all we need to solve here. dlopen()'ing the proper file takes care of that, > > the symbol version number check afterward is performed just in case. > > Interesting. These leftovers probably wouldn't be there if it wasn't > versioned in the first place. :-) Seriously, we can't assume users will do everything using neat packages and may run an unfortunate "make install" from the DPDK source tree without noticing they wrecked their system. Someone will have to mop the ensuing but preventable bug reports. > > > > > Given the added complexity, is there really a problem with simple version > > > > > numbers we increment every time something gets modified? (Note this is > > > > > already how our .map files work, they're not generated automatically) > > > > > > > > Our map files show the major version where a symbol was introduced. > > > > It is simple because no symbol can be introduced in a minor version. > > > > > > > > > How about keeping things as is? > > > > > > I don't really see the need of unique filenames. The next patch is > > > already leveraging RTE_EAL_PMD_PATH, which if versioned should be > > > enough for this, no? > > > > As you said, "if" versioned. As an undocumented empty string by default, > > there's no way to be sure. Leaving the PMD version its internal but > > (unfortunately) exposed bits will certainly prevent mistakes. > > > > > > You are using 18.02.1 while it is introduced in 18.02.0. > > > > If you don't want to correlate the .so version number with DPDK version > > > > number, maybe that 1, 2, 3 would be a simpler choice (less confusing). > > > > > > +1 > > > > Then are you fine with the "18.02.0" suffix? > > Not really, sorry. It was more for the "1, 2, 3" sequence or tying it > to dpdk version. > > With the latest replies, I don't think the reasoning is enough to > justify these extra checks, but I won't oppose to including it. 18.02.0 makes it tied to the current release number, so I guess we agree. The idea for now is this part remains tied to the DPDK release. If a new ABI version is needed in a subsequent commit, the initial part gets bumped to the current WIP DPDK release (say, 42.02.0). If subsequent intermediate commits break the glue ABI, a fourth digit is added (e.g. 42.02.0.1). This role is currently held by the third digit but since there's a confusion with DPDK revisions, it won't be used internally by the PMD. Hopefully this fourth digit will remain unused (otherwise I can add as many digits as necessary to make it acceptable, I'll then re-consider the SHA1 idea :) -- Adrien Mazarguil 6WIND