netdev - Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACKFLi=062ODBJ5yoR5Pe6hvCuOXyJ-reoUp7HhrbdB0nvstPQ@mail.gmail.com>
Date:   Tue, 30 Jun 2020 12:42:28 -0700
From:   Michael Chan <michael.chan@...adcom.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Miller <davem@...emloft.net>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set
 indirection table.

On Tue, Jun 30, 2020 at 12:05 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 29 Jun 2020 17:38:33 -0700 Michael Chan wrote:
> > On Mon, Jun 29, 2020 at 5:06 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > > On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> > > > With the new infrastructure in place, we can now support the setting of
> > > > the indirection table from ethtool.
> > > >
> > > > The user-configured indirection table will need to be reset to default
> > > > if we are unable to reserve the requested number of RX rings or if the
> > > > RSS table size changes.
> > > >
> > > > Signed-off-by: Michael Chan <michael.chan@...adcom.com>
> > >
> > > Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
> > > requested a RSS mapping, if it can't be maintained driver should
> > > return an error from the operation which attempts to change the ring
> > > count.
> >
> > Right.  In this case the user has requested non default RSS map and is
> > now attempting to change rings.  We have a first level check by
> > calling bnxt_check_rings().  Firmware will tell us if the requested
> > rings are available or not.  If not, we will return error and the
> > existing rings and RSS map will be kept.  This should be the expected
> > outcome in most cases.
> >
> > In rare cases, firmware can return success during bnxt_check_rings()
> > but during the actual ring reservation, it fails to reserve all the
> > rings it promised were available earlier.  In this case, we fall back
> > and accept the fewer rings and set the RSS map to default.  I have
> > never seen this scenario but we need to put the code in just in case
> > it happens.  It should be rare.
>
> What's the expected application flow? Every time the application
> makes a change to NIC settings it has to re-validate that some of
> the previous configuration didn't get lost? I don't see the driver
> returning the error if FW gave it less rings than requested. There
> isn't even a warning printed..

In bnxt_set_channels(), if bnxt_check_rings() returns error, we will
print a warning and return error.  This applies whether we have a user
defined RSS map or not.

If the RSS table size changes (only newer chips will change the RSS
table size when the rings cross some thresholds), the current code
always goes back to default RSS map.  But I think you prefer to keep
the user-defined RSS map and the rings and return error from
bnxt_set_channels().  This I can easily change.

I think I misunderstood your original question.  There is another code
path that will change the RSS map to default if we cannot reserve the
number of rings from firmware that were promised earlier from
bnxt_check_rings().  I was referring to this code path earlier.

>
> I'd prefer if the driver wrapped the rss indexes, and printed a
> warning, but left the config intact. And IMO set_channels should
> return an error.

Right.  I think it is clear now.  I will make this change in v2 plus
the other feedback you provided.  Thanks.