netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Bhargava Chenna Marreddy <bhargava.marreddy@broadcom.com>
Cc: davem@davemloft.net, edumazet@google.com, pabeni@redhat.com,
	andrew+netdev@lunn.ch, horms@kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, michael.chan@broadcom.com,
	pavan.chebbi@broadcom.com, vsrama-krishna.nemani@broadcom.com,
	vikas.gupta@broadcom.com,
	Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
Subject: Re: [v7, net-next 06/10] bng_en: Allocate packet buffers
Date: Mon, 15 Sep 2025 14:52:06 -0700	[thread overview]
Message-ID: <20250915145206.74a59699@kernel.org> (raw)
In-Reply-To: <CANXQDtaB7HcSujG1R9i90YUB6PdOin4=CsKzGvNX6tGMw8n+mw@mail.gmail.com>

On Mon, 15 Sep 2025 23:26:07 +0530 Bhargava Chenna Marreddy wrote:
> > You should have some sort of minimal fill level of the Rx rings.
> > Right now ndo_open will succeed even when Rx rings are completely empty.
> > Looks like you made even more functions void since v6, this is going in  
> I changed those functions to void only because in this patchset they can’t fail.
> > the wrong direction. Most drivers actually expect the entire ring to be
> > filled. You can have a partial fill, but knowing bnxt I'm worried the
> > driver will actually never try to fill the rings back up.  
> I believe the driver should return an error if any buffer allocation
> fails and handle the unwinding accordingly.

Yes, that's also my preference. I think allowing Rx buffer lists to not
be completely filled is okay if the driver author prefers that, but in
that case there needs to be some minimal "fill level" which makes the
device operational.

Speaking of Rx fill -- bnxt drops packets when it can't allocate a
replacement buffer. This used to be the recommended way of handling
allocation failures years ago. In modern drivers I believe it's better
to let the queue run dry and have a watchdog / service tasks which
periodically checks for complete depletion and kicks NAPI in.

Getting constantly interrupted with new packets when machine is trying
to recover from a hard OOM is not very helpful.

That's just a future note, I don't think this series itself contains
much of Rx.

  reply	other threads:[~2025-09-15 21:52 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-11 19:34 [v7, net-next 00/10] Add more functionality to BNGE Bhargava Marreddy
2025-09-11 19:34 ` [v7, net-next 01/10] bng_en: make bnge_alloc_ring() self-unwind on failure Bhargava Marreddy
2025-09-16 15:12   ` Simon Horman
2025-09-18  9:50     ` Bhargava Chenna Marreddy
2025-09-18 19:06       ` Simon Horman
2025-09-11 19:34 ` [v7, net-next 02/10] bng_en: Add initial support for RX and TX rings Bhargava Marreddy
2025-09-11 19:34 ` [v7, net-next 03/10] bng_en: Add initial support for CP and NQ rings Bhargava Marreddy
2025-09-16 14:54   ` Simon Horman
2025-09-18  9:40     ` Bhargava Chenna Marreddy
2025-09-11 19:34 ` [v7, net-next 04/10] bng_en: Introduce VNIC Bhargava Marreddy
2025-09-11 19:35 ` [v7, net-next 05/10] bng_en: Initialise core resources Bhargava Marreddy
2025-09-16 15:45   ` Simon Horman
2025-09-18 10:49     ` Bhargava Chenna Marreddy
2025-09-11 19:35 ` [v7, net-next 06/10] bng_en: Allocate packet buffers Bhargava Marreddy
2025-09-14 20:31   ` Jakub Kicinski
2025-09-15 17:56     ` Bhargava Chenna Marreddy
2025-09-15 21:52       ` Jakub Kicinski [this message]
2025-09-11 19:35 ` [v7, net-next 07/10] bng_en: Allocate stat contexts Bhargava Marreddy
2025-09-11 19:35 ` [v7, net-next 08/10] bng_en: Register rings with the firmware Bhargava Marreddy
2025-09-16 15:51   ` Simon Horman
2025-09-18 10:41     ` Bhargava Chenna Marreddy
2025-09-18 19:06       ` Simon Horman
2025-09-17 20:14   ` [External] : " ALOK TIWARI
2025-09-11 19:35 ` [v7, net-next 09/10] bng_en: Register default VNIC Bhargava Marreddy
2025-09-17 20:18   ` ALOK TIWARI
2025-09-19  6:28     ` Bhargava Chenna Marreddy
2025-09-11 19:35 ` [v7, net-next 10/10] bng_en: Configure " Bhargava Marreddy

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=20250915145206.74a59699@kernel.org \
    --to=kuba@kernel.org \
    --cc=andrew+netdev@lunn.ch \
    --cc=bhargava.marreddy@broadcom.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michael.chan@broadcom.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=pavan.chebbi@broadcom.com \
    --cc=rajashekar.hudumula@broadcom.com \
    --cc=vikas.gupta@broadcom.com \
    --cc=vsrama-krishna.nemani@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).