netdev - Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250616134828.703eafe5@kernel.org>
Date: Mon, 16 Jun 2025 13:48:28 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Michael Chan <michael.chan@...adcom.com>
Cc: Simon Horman <horms@...nel.org>, davem@...emloft.net,
 netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 andrew+netdev@...n.ch, pavan.chebbi@...adcom.com,
 andrew.gospodarek@...adcom.com, David Wei <dw@...idwei.uk>
Subject: Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS
 contexts on queue reset

On Mon, 16 Jun 2025 10:40:27 -0700 Michael Chan wrote:
> On Mon, Jun 16, 2025 at 6:39 AM Simon Horman <horms@...nel.org> wrote:
> > On Fri, Jun 13, 2025 at 04:18:41PM -0700, Michael Chan wrote:  
> > > From: Pavan Chebbi <pavan.chebbi@...adcom.com>
> > >
> > > The commit under the Fixes tag below which updates the VNICs' RSS
> > > and MRU during .ndo_queue_start(), needs to be extended to cover any
> > > non-default RSS contexts which have their own VNICs.  Without this
> > > step, packets that are destined to a non-default RSS context may be
> > > dropped after .ndo_queue_start().  
> >
> > This patch seems to be doing two things:
> > 1. Addressing the bug described above
> > 2. Implementing the optimisation below
> >
> > As such I think it would be best split into two patches.
> > And I'd lean towards targeting the optimisation at net-next
> > since "this scheme is just an improvement".  
> 
> The original fix (without the improvement) was rejected by Jakub and
> that's why we are improving it here.
> 
> Jakub, what do you think?

I think the phrasing of the commit message could be better, but the fix
is correct as is. We were shutting down just the main vNIC, now we shut
down all the vNICs to which the queue belongs.

It's not an "optimization" in the sense of an improvement to status quo,
IIUC Pavan means that shutting down the vNIC is still not 100% correct
for single queue reset, but best we can do with current FW. If we were
to split this into 2 changes, I don't think those changes would form a
logical progression: reset vNIC 0 (current) -> reset all (net) -> reset
the correct set of vNICs (net-next).. ?

I'd chalk this up to people writing sassy / opinion-tainted commit
messages after reviewers disagree with their initial implementation. 
I tend not to fight it :)