DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dan Gora <dg@adax.com>
To: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Cc: Luca Boccassi <bluca@debian.org>, "dev@dpdk.org" <dev@dpdk.org>,
	 David Marchand <david.marchand@redhat.com>,
	Jerin Jacob <jerinjacobk@gmail.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] eal: resolve getentropy at run time for random seed
Date: Mon, 4 May 2020 11:13:50 -0300
Message-ID: <CAGyogRbFEfbp1OU9fWwauMdup_h4_fD286T9_L7AgKm1O8T_-A@mail.gmail.com> (raw)
In-Reply-To: <7e0c2b55-e842-d5a2-017e-d85194e37452@ericsson.com>

On Mon, May 4, 2020 at 5:04 AM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:

> >> so what you are effectively asking is to
> >> double the size of the support matrix, and for the project to ensure
> >> that every single dependency can always be built against a new version
> >> and used against an older one.
> > No, this is not correct.  The size of the support/test matrix is
> > exactly the same, what changes is the number of build systems that
> > have to be supported.
> >
> > Before my patches you had to test targets with:
> >
> > no rdseed and no getentropy
> > rdseed and no getentropy
> > no rdseed and getentropy
> > rdseed and getentropy.
> rdseed will go away eventually, when all reasonable systems have
> getentropy(). That might well take a couple of years.

So then why did you include it in the first place?

> > For each of these test cases you had to have a separate build system
> > to generate a different binary (let's say app/test).
> >
> > Oh and you also had to test with building with make and building with
> > meson because getentropy was only supported with meson.
> >
> > Now, with my patches, you can build 'app/test' on *any* of these
> > systems, and use make _or_ meson, and it will run exactly the same
> > way as if you compiled it on the matching test system.
> >
> > That is the difference.
> >
> > Let's take another example:  Say I have an application which I have to
> > support on RHEL 7.7 and 7.5.  7.7 has getentropy, 7.5 does not (ignore
> > the rdseed case for now).
> >
> > So I am faced with:
> > 1) Build on the RHEL 7.7 system.  This allows the app to use
> > getentropy(), but it  cannot run on 7.5.
> > 2) Build on the RHEL 7.5 system.  This allows the app to run on 7.5
> > and 7.7, but only uses the TSC counter.
> > 3) Build on both systems.  Ok, now I have to build and support two
> > separate binaries.
> > 4) Determine if  getentropy() exists at run time or use /dev/urandom.
> > Now I can build on either system and it will use getentropy() on 7.7
> > and the TSC counter on 7.5.
> You build against the oldest libc supported. The rest of the
> dependencies, you carry with your application.

What does this even mean?   That I'm supposed to distribute a proper
copy of binutils/glibc for each Linux distribution that we have to
support?  Am I supposed to distribute that as source and have our
customers compile it from source or do I have to maintain a proper
version of glibc for every linux distribution that we have to support?

> Again, I believe this to
> be standard practice, and has been so for ages. Containers are really an
> extension of this method, but it was common pattern when commercial
> applications were more common (especially true for things like Solaris-
> or HP-UX-based apps).

This was not at all true for Solaris or HPUX.  Software that we wrote
worked on every release of Solaris from 2.6 up until Solaris 9.  We
had some 8 glorious years of binary stability.  And there were not 15
different distributions of Solaris or HPUX that we had to support.

> You didn't even care about getentropy(), so I
> can't understand how this is a problem.

Because the patch which introduced getentropy() made it so that if I
build on the wrong machine the binary will not work everywhere because
of this unnecessary dependency on glibc 2.25.  If I compile on a
machine which happened to have rdseed, suddenly my application would
not even start on a machine which did not have rdseed.  That was not a
problem until 19.08.

> > How is #3 "better" than #4?  You still have to test on 7.7 and 7.5.
> > The amount of work in testing has not changed at all, only the number
> > of build systems and binary versions that you have to maintain.
> >
> > And I'm not saying that this has to be a hard and fast rule for
> > everything for now and forever.  I am fixing a small function which is
> > used exactly once.  The original patch which introduced getentropy()
> > in v19.08 created an extra dependency that the target system have
> > glibc 2.25, _even if it does not use this function at all_.
> >
> > That said, it seems to me to be a good idea to remove as many build
> > time dependencies as possible and use things like
> > rte_cpu_get_flag_enabled() to determine the best code path at run time
> > (that is, initialization time, not in the fastpath obviously).  It
> > just makes life a lot easier... If the compiler can generate the code,
> > then it should be possible in many cases.
> >
> >>>>>> This is especially true when dealing with RNG APIs, where the tiniest
> >>>>>> and most innocent-looking mistake could have disastrous consequences.
> >>>>> This does not change the functionality of the RNG at all.  It just
> >>>>> makes it work in the way that it was intended.  These changes were
> >>>>> only introduced into 19.08, so they are not historical artifacts or
> >>>>> anything.
> >>>> It's reimplementing a syscall using a different interface which has
> >>>> different semantics. A small mistake there is going to cost us dearly.
> >>> The code was copied almost verbatim from the getentropy() function in
> >>> glibc, it's hard to see it going that wrong there.  The code can be
> >>> tested using the same test cases that the original patch used.  I
> >>> don't see how there is a difference in test coverage here.
> >> And who's going to keep it updated as the implementation in glibc
> >> changes, if/when a bug is found there?
> > What happens when any bug is found in DPDK... It gets fixed.  Why is
> > this any different?
> >
> >> Also, glibc is LGPL2.1 licensed, which means copying almost verbatim a
> >> non-trivial piece of code can possibly result in librte_eal as a whole
> >> to be covered under the terms of LGPL2.1 (IANAL disclaimer). This is
> >> unlikely to be well received, given it's BSD-3-clause now.
> > Well, verbatim, is an exaggeration and IANAL either, but I would
> > consider what was written to be general enough to not really run into
> > any problems.  If others disagree, the v3 patch is there waiting too..
> >
> >>> If it's such a big deal to use /dev/urandom, then what about my v3
> >>> patch which used dlopen()/dlsym() to try to find getentropy()?  Is
> >>> that not then acceptable?   dlopen/dlsym() are used in several placed
> >>> in DPDK.
> >> I don't have a problem with that specifically, but it needs to be clear
> >> to the maintainers that what you are effectively asking is for all
> >> dependencies to support runtime-downgrading transparently and out of
> >> the box.
> > Again, I am not asking for all build time dependencies to be
> > eliminated, just this one, which was only introduced in v19.08, which
> > creates a dependency on glibc 2.25, even if the application does not
> > use the RNG at all.
> There's no new dependency if you build against an older glibc version.

Because the point is that now I have to care.  Now I have to check and
make sure that the machine I build on has the right glibc version. I
have to find a machine which does not have rdseed on it in order to
build my application.  Why should I even have to care about this?

Why are you arguing against all this now?  You suggested to use
/dev/urandom.  You used it in your original patches for these changes.
You said these changes were "a good idea".  Why is it suddenly a bad
idea now?

> > This is already done in numerous places in the DPDK. See
> > aesni_gcm_create().  See rte_hash_crc_set_alg().  See
> > rte_hash_create().  They all check to see if certain CPU flags are
> > supported and use the appropriate function.  This is not "downgrading"
> > anything.  It is using the best available function that is supported
> > on the target system, not the build system.  That's exactly what this
> > (or the v3) patch does.
> >
> >> This comes with a huge additional cost, and I do not believe
> >> it is supported at the moment.
> > It does not come at any additional cost really, all it does is reduce
> > the number of build systems which have to be supported.  It is exactly
> > the same idea as my first patch which checks for the RDSEED
> > instruction at run time and if the target system does not support it
> > "downgrades" to use the TSC counter.  Except now this is done at
> > run-time, not at build time.
> >
> The make build is going away completely.

Well it's not going away for at least a year or two and that has
nothing to do with the issues at hand here.  And I sure hope that
someone actually writes some documentation for how to build third
party applications using DPDK and meson before that happens because
currently there is none.  But it's pretty clear the DPDK community
doesn't really care about lowly "third party developers", so I'm sure
that will not happen.


  reply	other threads:[~2020-05-04 14:14 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-21 19:54 [dpdk-dev] [PATCH 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-21 19:54 ` [dpdk-dev] [PATCH 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-22  8:22   ` Mattias Rönnblom
2020-04-21 19:54 ` [dpdk-dev] [PATCH 2/2] eal: resolve getentropy " Dan Gora
2020-04-21 21:03   ` Stephen Hemminger
2020-04-21 21:08     ` Dan Gora
2020-04-22  8:28   ` Mattias Rönnblom
2020-04-22 17:44     ` Dan Gora
2020-04-22 20:14       ` Mattias Rönnblom
2020-04-22 20:35         ` Dan Gora
2020-04-23 10:04           ` Luca Boccassi
2020-04-23 17:38             ` Dan Gora
2020-04-27 12:44               ` Luca Boccassi
2020-04-27 16:57                 ` Dan Gora
2020-04-30  8:41                   ` Luca Boccassi
2020-04-30 20:43                     ` Dan Gora
2020-05-01 10:33                       ` Luca Boccassi
2020-05-01 21:05                         ` Dan Gora
2020-05-04  8:04                           ` Mattias Rönnblom
2020-05-04 14:13                             ` Dan Gora [this message]
2020-05-04 14:19                               ` Dan Gora
2020-06-02  5:10                                 ` Dan Gora
2020-06-09 15:37                                   ` Dan Gora
2020-06-10  8:15                                     ` Thomas Monjalon
2020-06-10  8:33                                       ` Luca Boccassi
2020-06-10  8:07                               ` Thomas Monjalon
2020-04-23 12:36           ` Mattias Rönnblom
2020-04-23 17:27             ` Dan Gora
2020-04-21 20:41 ` [dpdk-dev] [PATCH v2 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-21 20:41   ` [dpdk-dev] [PATCH v2 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-21 20:41   ` [dpdk-dev] [PATCH v2 2/2] eal: resolve getentropy " Dan Gora
2020-04-22 18:15 ` [dpdk-dev] [PATCH v3 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-22 18:15   ` [dpdk-dev] [PATCH v3 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-22 18:15   ` [dpdk-dev] [PATCH v3 2/2] eal: resolve getentropy " Dan Gora
2020-04-22 23:42 ` [dpdk-dev] [PATCH v4 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-22 23:42   ` [dpdk-dev] [PATCH v4 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-22 23:42   ` [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial " Dan Gora
2020-04-23  2:39     ` Stephen Hemminger
2020-04-23 17:42       ` Dan Gora
2020-06-29  9:30     ` Mattias Rönnblom
2020-06-29 17:57       ` Dan Gora
2020-06-29 20:57         ` Mattias Rönnblom
2020-06-29  9:32   ` [dpdk-dev] [PATCH v4 0/2] eal: choose initial PRNG seed source at runtime Mattias Rönnblom
2020-06-29 18:01     ` Dan Gora
2020-06-29 18:04       ` Dan Gora
2020-06-29 21:05       ` Mattias Rönnblom
2020-06-29 21:14         ` Dan Gora

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGyogRbFEfbp1OU9fWwauMdup_h4_fD286T9_L7AgKm1O8T_-A@mail.gmail.com \
    --to=dg@adax.com \
    --cc=bluca@debian.org \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jerinjacobk@gmail.com \
    --cc=mattias.ronnblom@ericsson.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git