From: Jakub Kicinski <kuba@kernel•org>
To: Mina Almasry <almasrymina@google•com>
Cc: davem@davemloft•net, netdev@vger•kernel.org, edumazet@google•com,
pabeni@redhat•com, andrew+netdev@lunn•ch, horms@kernel•org,
donald.hunter@gmail•com, sdf@fomichev•me, dw@davidwei•uk,
asml.silence@gmail•com, ap420073@gmail•com, jdamato@fastly•com,
dtatulea@nvidia•com, michael.chan@broadcom•com
Subject: Re: [RFC net-next 11/22] net: allocate per-queue config structs and pass them thru the queue API
Date: Fri, 25 Apr 2025 16:24:37 -0700 [thread overview]
Message-ID: <20250425162437.45765bc0@kernel.org> (raw)
In-Reply-To: <CAHS8izN5ZjCmBC-+_p0kLFNo5hexEG=GKfk7Jd7w4wokcw2F3w@mail.gmail.com>
On Wed, 23 Apr 2025 14:17:30 -0700 Mina Almasry wrote:
> > @@ -129,6 +136,10 @@ struct netdev_stat_ops {
> > *
> > * @ndo_queue_mem_size: Size of the struct that describes a queue's memory.
> > *
> > + * @ndo_queue_cfg_defaults: (Optional) Populate queue config struct with
> > + * defaults. Queue config structs are passed to this
> > + * helper before the user-requested settings are applied.
> > + *
> > * @ndo_queue_mem_alloc: Allocate memory for an RX queue at the specified index.
> > * The new memory is written at the specified address.
> > *
> > @@ -146,12 +157,17 @@ struct netdev_stat_ops {
> > */
> > struct netdev_queue_mgmt_ops {
> > size_t ndo_queue_mem_size;
> > + void (*ndo_queue_cfg_defaults)(struct net_device *dev,
> > + int idx,
> > + struct netdev_queue_config *qcfg);
> > int (*ndo_queue_mem_alloc)(struct net_device *dev,
> > + struct netdev_queue_config *qcfg,
> > void *per_queue_mem,
> > int idx);
> > void (*ndo_queue_mem_free)(struct net_device *dev,
> > void *per_queue_mem);
> > int (*ndo_queue_start)(struct net_device *dev,
> > + struct netdev_queue_config *qcfg,
> > void *per_queue_mem,
> > int idx);
> > int (*ndo_queue_stop)(struct net_device *dev,
>
> Doesn't the stop call need to return the config of the queue that was
> stopped? Otherwise what do you pass to the start command if you want
> to restart the old queue?
As you say below, I expect driver to retain the config within qmem.
> In general I thought what when we extend the queue API to handle
> configs, the config of each queue would be part of the per_queue_mem
> attribute, which is now a void *. Because seems to me the config needs
> to be passed around with the per_queue_mem to all the functions?
> Maybe.
>
> I imagined you'd create a new wrapper struct that is the per queue
> mem, and that one will contain a void * that is driver specific
> memory, and a netdev_queue_config * inside of it as well, then the
> queue API will use the new struct instead of void * for all the
> per_queue_mem. At least that's what I was thinking.
That sounds nice from the API perspective, but I was worried about
perf impact. Some driver folks count each cycle and we may be mixing
cache cold and cache hot data in the config. At the very least not
all drivers will support all fields. So my gut feeling was that driver
authors will want to assign the fields to their own struct members,
anyway. Perhaps something we can revisit once we have more mileage?
> > + maxqs = max(dev->num_rx_queues, dev->num_tx_queues);
> > + cfg->qcfg = kcalloc(maxqs, sizeof(*cfg->qcfg), GFP_KERNEL_ACCOUNT);
>
> I just thought of this, but for devices that can new rx/tx queues on
> the fly, isn't num_rx_queues dynamically changing? How do you size
> this qcfg array in this case?
>
> Or do they go through a full driver reset everytime they add a queue
> which reallocates dev->num_rx_queues?
Yes, good question. Easiest way to deal with that I thought of was
to act like old memory school management. "normal" queues allocate
indexes 0 -> n, "dynamic" queues allocate n -> 0. Like stack vs heap
growth. And we need to patch up all this code that naively checks
queue ID vs num_real, we'll need a bitmap or a member in the structs.
>>> + __netdev_queue_config(dev, rxq_idx, &qcfg, false);
>
> Ah, on error, you reset the queue to its defaults, which I'm not sure
> is desirable. I think we want to restart the queue with whatever
> config it had before.
Not defaults to be clear. The last argument here (false) tells
the config code to use the _current_ config not the pending one.
So we should be reverting to what's currently installed.
next prev parent reply other threads:[~2025-04-25 23:24 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-21 22:28 [RFC net-next 00/22] net: per-queue rx-buf-len configuration Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 01/22] docs: ethtool: document that rx_buf_len must control payload lengths Jakub Kicinski
2025-04-22 16:19 ` David Wei
2025-04-22 19:48 ` Joe Damato
2025-04-23 20:08 ` Mina Almasry
2025-04-25 22:50 ` Jakub Kicinski
2025-04-25 23:20 ` Joe Damato
2025-04-21 22:28 ` [RFC net-next 02/22] net: ethtool: report max value for rx-buf-len Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 03/22] net: use zero value to restore rx_buf_len to default Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 04/22] net: clarify the meaning of netdev_config members Jakub Kicinski
2025-04-22 19:57 ` Joe Damato
2025-04-21 22:28 ` [RFC net-next 05/22] net: add rx_buf_len to netdev config Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 06/22] eth: bnxt: read the page size from the adapter struct Jakub Kicinski
2025-04-23 20:35 ` Mina Almasry
2025-04-25 22:51 ` Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 07/22] eth: bnxt: set page pool page order based on rx_page_size Jakub Kicinski
2025-04-22 15:32 ` Stanislav Fomichev
2025-04-22 15:52 ` Jakub Kicinski
2025-04-22 17:27 ` Stanislav Fomichev
2025-04-21 22:28 ` [RFC net-next 08/22] eth: bnxt: support setting size of agg buffers via ethtool Jakub Kicinski
2025-04-23 21:00 ` Mina Almasry
2025-04-25 22:58 ` Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 09/22] net: move netdev_config manipulation to dedicated helpers Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 10/22] net: reduce indent of struct netdev_queue_mgmt_ops members Jakub Kicinski
2025-04-23 21:04 ` Mina Almasry
2025-04-21 22:28 ` [RFC net-next 11/22] net: allocate per-queue config structs and pass them thru the queue API Jakub Kicinski
2025-04-23 21:17 ` Mina Almasry
2025-04-25 23:24 ` Jakub Kicinski [this message]
2025-04-21 22:28 ` [RFC net-next 12/22] net: pass extack to netdev_rx_queue_restart() Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 13/22] net: add queue config validation callback Jakub Kicinski
2025-04-22 15:49 ` Stanislav Fomichev
2025-04-22 20:16 ` Joe Damato
2025-04-21 22:28 ` [RFC net-next 14/22] eth: bnxt: always set the queue mgmt ops Jakub Kicinski
2025-04-22 15:50 ` Stanislav Fomichev
2025-04-22 20:18 ` Joe Damato
2025-04-21 22:28 ` [RFC net-next 15/22] eth: bnxt: store the rx buf size per queue Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 16/22] eth: bnxt: adjust the fill level of agg queues with larger buffers Jakub Kicinski
2025-04-22 16:13 ` Stanislav Fomichev
2025-04-21 22:28 ` [RFC net-next 17/22] netdev: add support for setting rx-buf-len per queue Jakub Kicinski
2025-04-22 16:15 ` Stanislav Fomichev
2025-04-25 23:41 ` Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 18/22] net: wipe the setting of deactived queues Jakub Kicinski
2025-04-22 16:21 ` Stanislav Fomichev
2025-04-25 23:42 ` Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 19/22] eth: bnxt: use queue op config validate Jakub Kicinski
2025-04-23 10:00 ` Dragos Tatulea
2025-04-23 13:46 ` Jakub Kicinski
2025-04-23 14:24 ` Dragos Tatulea
2025-04-23 15:33 ` Jakub Kicinski
2025-06-12 11:56 ` Dragos Tatulea
2025-06-12 14:10 ` Jakub Kicinski
2025-06-12 15:52 ` Dragos Tatulea
2025-06-12 22:30 ` Jakub Kicinski
2025-06-13 19:02 ` Dragos Tatulea
2025-06-13 23:16 ` Jakub Kicinski
2025-06-17 12:36 ` Dragos Tatulea
2025-04-21 22:28 ` [RFC net-next 20/22] eth: bnxt: support per queue configuration of rx-buf-len Jakub Kicinski
2025-04-21 22:28 ` [RFC net-next 21/22] selftests: drv-net: add helper/wrapper for bpftrace Jakub Kicinski
2025-04-22 16:36 ` Stanislav Fomichev
2025-04-22 16:39 ` Stanislav Fomichev
2025-06-25 12:23 ` Breno Leitao
2025-04-21 22:28 ` [RFC net-next 22/22] selftests: drv-net: add test for rx-buf-len Jakub Kicinski
2025-04-22 17:06 ` Stanislav Fomichev
2025-04-25 23:52 ` Jakub Kicinski
2025-04-23 20:02 ` [RFC net-next 00/22] net: per-queue rx-buf-len configuration Mina Almasry
2025-04-25 23:55 ` Jakub Kicinski
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=20250425162437.45765bc0@kernel.org \
--to=kuba@kernel$(echo .)org \
--cc=almasrymina@google$(echo .)com \
--cc=andrew+netdev@lunn$(echo .)ch \
--cc=ap420073@gmail$(echo .)com \
--cc=asml.silence@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=donald.hunter@gmail$(echo .)com \
--cc=dtatulea@nvidia$(echo .)com \
--cc=dw@davidwei$(echo .)uk \
--cc=edumazet@google$(echo .)com \
--cc=horms@kernel$(echo .)org \
--cc=jdamato@fastly$(echo .)com \
--cc=michael.chan@broadcom$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=sdf@fomichev$(echo .)me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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