[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190603160351.085daa91@cakuba.netronome.com>
Date: Mon, 3 Jun 2019 16:03:51 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: "Woodhouse, David" <dwmw@...zon.co.uk>
Cc: "Jubran, Samih" <sameehj@...zon.com>,
"Kiyanovski, Arthur" <akiyano@...zon.com>,
"Bshara, Saeed" <saeedb@...zon.com>,
"Tzalik, Guy" <gtzalik@...zon.com>,
"Matushevsky, Alexander" <matua@...zon.com>,
"Liguori, Anthony" <aliguori@...zon.com>,
"Saidi, Ali" <alisaidi@...zon.com>,
"Machulsky, Zorik" <zorik@...zon.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Wilson, Matt" <msw@...zon.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"Belgazal, Netanel" <netanel@...zon.com>,
"Bshara, Nafea" <nafea@...zon.com>,
"Herrenschmidt, Benjamin" <benh@...zon.com>,
Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH V2 net 00/11] Extending the ena driver to support new
features and enhance performance
On Mon, 3 Jun 2019 22:27:28 +0000, Woodhouse, David wrote:
> On Mon, 2019-06-03 at 14:32 -0700, Jakub Kicinski wrote:
> > On Mon, 3 Jun 2019 17:43:18 +0300, sameehj@...zon.com wrote:
> > > * net: ena: ethtool: add extra properties retrieval via get_priv_flags (2/11):
> > > * replaced snprintf with strlcpy
> > > * dropped confusing error message
> > > * added more details to the commit message
> >
> > I asked you to clearly state that you are using the blindly passing
> > this info from the FW to user space. Stating that you "retrieve" it
> > is misleading.
>
> Yes, we should be very clear about that.
>
> > IMHO it's a dangerous precedent, you're extending kernel's uAPI down to
> > the proprietary FW of the device. Now we have no idea what the flags
> > are, so we can't refactor and create common APIs among drivers, or even
> > use the same names. We have no idea what you're exposing.
>
> Yes, that should be documented very clearly too, and we should
> absolutely make sure that anything that makes sense for other devices
> is considered for making a common API.
>
> However, the deployment environment for ENA is kind of weird — we get
> to upgrade the *firmware*, while guest kernels might get updated only
> once a decade. So the passthrough you're objecting to, actually gives
> us fairly much the only viable way to offer many users the tuning
> options they need — or at least to experiment and establish whether
> they *do* need them or not, and thus if we should turn them into a
> "real" generic property.
>
> You do have a valid concern, but I think we can handle it, and I
> suspect that the approach taken here does make sense. Let's just make
> it explicitly clear, and also document the properties that we expect to
> be exposing, for visibility.
Thank you.
It's generally easier to push FW updates, also to enterprises, rather
than rolling out full new kernels. People are less concerned with FW
updates, because the only thing those can break is the NIC. Some
customers also run their own modified kernels. I'd dispute the fact
that Amazon is so very special.
I don't dispute that this is convenient for Amazon and your FW team.
However, what's best for the vendors differs here from what's good for
Open Source (the FW is proprietary) and IMHO health of Linux ecosystem
in general.
Any "SmartNIC" vendor has temptation of uAPI-level hand off to the
firmware (including my employer), we all run pretty beefy processors
inside "the NIC" after all. The device centric ethtool configuration
can be implemented by just forwarding the uAPI structures as they are
to the FW. I'm sure Andrew and others who would like to see Linux
takes more control over PHYs etc. would not like this scenario, either.
To address your specific points, let's be realistic, after initial
submission it's unlikely the features will be updated in a timely
manner anywhere else than perhaps some Amazon's own docs. Can you
be more specific on what those tuning options are today? There are
users who don't care to update their kernels for 10 years, yet they
care about some minor tuning option of the NIC?
Powered by blists - more mailing lists