From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0D584A00BE; Mon, 4 May 2020 16:14:36 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E0C0E1D176; Mon, 4 May 2020 16:14:35 +0200 (CEST) Received: from mail-io1-f67.google.com (mail-io1-f67.google.com [209.85.166.67]) by dpdk.org (Postfix) with ESMTP id B0D6B1D167 for ; Mon, 4 May 2020 16:14:34 +0200 (CEST) Received: by mail-io1-f67.google.com with SMTP id f3so12434042ioj.1 for ; Mon, 04 May 2020 07:14:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=OpxZHdUUOW75AKpZe6RxcPdawjyrOCFrRm+hJhTxVDg=; b=DhvbGmrIgbGyvLtQg8AT5LG0hqYZDhA2U3bcKW5Zl2QCP78SLBj+NBfAZfa2p9rabn gGDvbUFasBaDFAlvGqbHoSmcSW0vpS3m8veX9M+IarWmL369IHaC8Aqu7yeeg67tkXOG LMEwO9s0qEMVRUYLUmTNd0MXVnBW6ziMCEoBhjTJVS7wzk12OU/xa0lRCmgnifjYMtZ5 LJBdcnzjv5XzvYJoZmXLC4dzn037fJRA3S9XubeUjdImOgTww4C6X5jSuixEXcV/gsuI AQbPeCsPckRM/bj33bQ3q6azD4QKUxeao+dYbYLQTZXBE4x6uUtvvyHI/BO89WLBpWRd pYKQ== X-Gm-Message-State: AGi0Pua+9g5U5fcPDlvOBC1CcrHnQihlhpAOq80lSmMLp0uZXcVqPMZ5 73SprxlQcS7fPS4lWJdmdK3UD/qsyjBnPafvUK8= X-Google-Smtp-Source: APiQypKlBvisXvoih4UxA9ch7Tx8MYRug74VvRcEenbZ1GPN0X/l7ICO+nlgDRSEFuCXwud0w2mGXGed4UX8nutKylk= X-Received: by 2002:a02:cbba:: with SMTP id v26mr14784119jap.14.1588601666476; Mon, 04 May 2020 07:14:26 -0700 (PDT) MIME-Version: 1.0 References: <20200421195446.1730-1-dg@adax.com> <20200421195446.1730-3-dg@adax.com> <5df15087-781e-d27f-b7d8-50b1b8cb0c94@ericsson.com> <80e5cf97-feae-c753-5c65-4f3b121729f3@ericsson.com> <9782db5b1e5d2dfdcfa8e52410fd166df5db938d.camel@debian.org> <7e0c2b55-e842-d5a2-017e-d85194e37452@ericsson.com> In-Reply-To: <7e0c2b55-e842-d5a2-017e-d85194e37452@ericsson.com> From: Dan Gora Date: Mon, 4 May 2020 11:13:50 -0300 Message-ID: To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Cc: Luca Boccassi , "dev@dpdk.org" , David Marchand , Jerin Jacob Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH 2/2] eal: resolve getentropy at run time for random seed 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, May 4, 2020 at 5:04 AM Mattias R=C3=B6nnblom 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 tini= est > >>>>>> and most innocent-looking mistake could have disastrous consequenc= es. > >>>>> 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 dearl= y. > >>> 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 clea= r > >> 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. thanks dan