From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Marcelo Ricardo Leitner <mleitner@redhat.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
"Van Haaren, Harry" <harry.van.haaren@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Shahaf Shuler <shahafs@mellanox.com>,
Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/mlx: version rdma-core glue libraries
Date: Tue, 6 Feb 2018 12:06:38 +0100 [thread overview]
Message-ID: <20180206110638.GS4256@6wind.com> (raw)
In-Reply-To: <20180205170619.GI27676@localhost.localdomain>
On Mon, Feb 05, 2018 at 03:06:19PM -0200, Marcelo Ricardo Leitner wrote:
> On Mon, Feb 05, 2018 at 04:54:55PM +0100, Adrien Mazarguil wrote:
> > 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:
> ...
> > > > > > > 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.
>
> It makes them equal, but not tied. If nobody patches it, when 18.02.1
> is out, the glue lib will still be 18.02.0.
Well this must be understood as "this plug-in implements 18.02.0's mlx4 glue
ABI", which remains true (and compatible) with subsequent DPDK releases as
long as the glue code is not updated.
Note this is no different from a single-digit suffix, which wouldn't be
updated either if the ABI isn't. Again, these initial digits are needed
because otherwise there is already a confusion with stable branches that
implement different ABIs and are therefore incompatible:
librte_pmd_mlx4_glue.so.17.02.1
librte_pmd_mlx4_glue.so.17.11.1
librte_pmd_mlx4_glue.so.18.02.0
With a single digit, all of them would be named "librte_pmd_mlx4_glue.so.1",
rendering versioning basically useless.
> > 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).
>
> I'll defer this to other project developers. This is more about a
> project standard than anything here. I could even argue that this glue
> should be named after the pmd lib, such as
> ./usr/local/lib/librte_pmd_mlx4_glue.so.1.1
> The fact of not providing the _glue.so symlink is enough to avoid
> others from linking against it. But it's more of a project standard
> than a technical decision, I guess, weather this lib is seen as a
> plugin or as a (private) library.
I think you nailed it, I call it a "plug-in" because dlopen() is manually
performed on it, however it's in fact a private library whose API is not
exposed and no application is supposed to use directly.
For this reason, while up to package maintainers, my suggestion is to not
install it in a public location like "/usr/local/lib" but configure
RTE_EAL_PMD_PATH to some DPDK-specific path, e.g. "/usr/share/dpdk/pmd",
which is possible since patch 4/4 of this series.
> Considering the versioning used for the PMD libs, such easy versioning
> is my preferred choice, FWIW.
Problem remains that the DPDK projects manages its own backports/stable
releases system instead of relying on package maintainers for that, so
properly versioning things from the beginning to avoid collisions is really
always a concern. Had backports not been a requirement in the first place,
I agree a single digit would have been enough.
My suggestion of using 18.02.0 (instead of 18.02.1) stands. It addresses
Thomas' concern by properly matching the DPDK release the ABI was last
updated for and mine for the backports issues mentioned above. Let's go with
that and move on.
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2018-02-06 11:06 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-02 15:16 [dpdk-dev] [PATCH v1 0/4] net/mlx: enhance rdma-core glue configuration Adrien Mazarguil
2018-02-02 15:16 ` [dpdk-dev] [PATCH v1 1/4] net/mlx: add debug checks to glue structure Adrien Mazarguil
2018-02-02 15:16 ` [dpdk-dev] [PATCH v1 2/4] net/mlx: fix missing includes for rdma-core glue Adrien Mazarguil
2018-02-02 15:16 ` [dpdk-dev] [PATCH v1 3/4] net/mlx: version rdma-core glue libraries Adrien Mazarguil
2018-02-02 15:16 ` [dpdk-dev] [PATCH v1 4/4] net/mlx: make rdma-core glue path configurable Adrien Mazarguil
2018-02-02 16:46 ` [dpdk-dev] [PATCH v2 0/4] net/mlx: enhance rdma-core glue configuration Adrien Mazarguil
2018-02-02 16:46 ` [dpdk-dev] [PATCH v2 1/4] net/mlx: add debug checks to glue structure Adrien Mazarguil
2018-02-05 12:27 ` Marcelo Ricardo Leitner
2018-02-05 13:31 ` Adrien Mazarguil
2018-02-02 16:46 ` [dpdk-dev] [PATCH v2 2/4] net/mlx: fix missing includes for rdma-core glue Adrien Mazarguil
2018-02-02 16:46 ` [dpdk-dev] [PATCH v2 3/4] net/mlx: version rdma-core glue libraries Adrien Mazarguil
2018-02-04 14:29 ` Thomas Monjalon
2018-02-05 11:24 ` Adrien Mazarguil
2018-02-05 12:13 ` Marcelo Ricardo Leitner
2018-02-05 12:24 ` Van Haaren, Harry
2018-02-05 12:43 ` Marcelo Ricardo Leitner
2018-02-05 12:58 ` Marcelo Ricardo Leitner
2018-02-05 13:44 ` Adrien Mazarguil
2018-02-05 14:16 ` Thomas Monjalon
2018-02-05 14:33 ` Adrien Mazarguil
2018-02-05 14:37 ` Marcelo Ricardo Leitner
2018-02-05 14:59 ` Adrien Mazarguil
2018-02-05 15:29 ` Marcelo Ricardo Leitner
2018-02-05 15:54 ` Adrien Mazarguil
2018-02-05 17:06 ` Marcelo Ricardo Leitner
2018-02-06 11:06 ` Adrien Mazarguil [this message]
2018-02-02 16:46 ` [dpdk-dev] [PATCH v2 4/4] net/mlx: make rdma-core glue path configurable Adrien Mazarguil
2018-02-02 16:52 ` [dpdk-dev] [PATCH v2 0/4] net/mlx: enhance rdma-core glue configuration Nélio Laranjeiro
2018-02-06 11:31 ` Shahaf Shuler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180206110638.GS4256@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=harry.van.haaren@intel.com \
--cc=mleitner@redhat.com \
--cc=nelio.laranjeiro@6wind.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).