netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Michael Chan <michael.chan@broadcom.com>
Cc: Simon Horman <horms@kernel.org>,
	davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com,
	pabeni@redhat.com, andrew+netdev@lunn.ch,
	pavan.chebbi@broadcom.com, andrew.gospodarek@broadcom.com,
	David Wei <dw@davidwei.uk>
Subject: Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset
Date: Mon, 16 Jun 2025 13:48:28 -0700	[thread overview]
Message-ID: <20250616134828.703eafe5@kernel.org> (raw)
In-Reply-To: <CACKFLimACdMBNZ2v-zpJ5=H1JdyWfLjN_0SqXkPe9waacq=GiQ@mail.gmail.com>

On Mon, 16 Jun 2025 10:40:27 -0700 Michael Chan wrote:
> On Mon, Jun 16, 2025 at 6:39 AM Simon Horman <horms@kernel.org> wrote:
> > On Fri, Jun 13, 2025 at 04:18:41PM -0700, Michael Chan wrote:  
> > > From: Pavan Chebbi <pavan.chebbi@broadcom.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 :)

  reply	other threads:[~2025-06-16 20:48 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-13 23:18 [PATCH net 0/3] bnxt_en: Bug fixes Michael Chan
2025-06-13 23:18 ` [PATCH net 1/3] bnxt_en: Fix double invocation of bnxt_ulp_stop()/bnxt_ulp_start() Michael Chan
2025-06-16 13:39   ` Simon Horman
2025-06-13 23:18 ` [PATCH net 2/3] bnxt_en: Add a helper function to configure MRU and RSS Michael Chan
2025-06-13 23:18 ` [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset Michael Chan
2025-06-16 13:38   ` Simon Horman
2025-06-16 17:40     ` Michael Chan
2025-06-16 20:48       ` Jakub Kicinski [this message]
2025-06-16 21:00         ` Michael Chan
2025-06-17 23:10 ` [PATCH net 0/3] bnxt_en: Bug fixes patchwork-bot+netdevbpf
  -- strict thread matches above, loose matches on Subject: below --
2025-05-19 20:41 [PATCH net 0/3] bnxt_en: 2 bug fixes Michael Chan
2025-05-19 20:41 ` [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset Michael Chan
2025-05-20 14:42   ` Simon Horman
2025-05-20 14:59   ` David Wei
2025-05-21  1:28   ` Jakub Kicinski
2025-05-21  1:38     ` Michael Chan
2025-05-21  1:51       ` Jakub Kicinski
2025-05-21  2:10         ` Michael Chan
2025-05-21  2:17           ` Jakub Kicinski
2025-05-21  2:29             ` Michael Chan
2025-05-22 11:01               ` David Wei
2025-05-22 15:26                 ` Jakub Kicinski
2025-05-27 15:37                   ` David Wei
2025-05-27 16:50                     ` Michael Chan

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=20250616134828.703eafe5@kernel.org \
    --to=kuba@kernel.org \
    --cc=andrew+netdev@lunn.ch \
    --cc=andrew.gospodarek@broadcom.com \
    --cc=davem@davemloft.net \
    --cc=dw@davidwei.uk \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=michael.chan@broadcom.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=pavan.chebbi@broadcom.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).