Queue configs and large buffer providers

We're going to give more control over rx buffer sizes to user space, and since we can't always rely on driver validation, let's sanitise it in page_pool_init() as well. Note that we only need to reject over MAX_PAGE_ORDER allocations for normal page pools, as current memory providers don't need to use the buddy allocator and must check the order on init.

Suggested-by Stanislav Fomichev <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Quoted 8 lines · Pavel Begunkov

We're going to give more control over rx buffer sizes to user space, and since we can't always rely on driver validation, let's sanitise it in page_pool_init() as well. Note that we only need to reject over MAX_PAGE_ORDER allocations for normal page pools, as current memory providers don't need to use the buddy allocator and must check the order on init.

Quoted 2 lines

Suggested-by Stanislav Fomichev <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Reviewed-by Mina Almasry <[email protected]>

I think I noticed an unrelated bug in this code and we need this fix?

```

net/core/page_pool.c +4 -2

index 343a6cac21e3..ba70569bd4b0 100644
@@ -287,8 +287,10 @@ static int page_pool_init(struct page_pool *pool,
         }
 
         if (pool->mp_ops) {
-               if (!pool->dma_map || !pool->dma_sync)
-                       return -EOPNOTSUPP;
+               if (!pool->dma_map || !pool->dma_sync) {
+                       err = -EOPNOTSUPP;
+                       goto free_ptr_ring;
+               }
 
                 if (WARN_ON(!is_kernel_rodata((unsigned long)pool->mp_ops))) {
                         err = -EFAULT;

```

From: Jakub Kicinski <[email protected]>

Document the semantics of the rx_buf_len ethtool ring param. Clarify its meaning in case of HDS, where driver may have two separate buffer pools.

The various zero-copy TCP Rx schemes we have suffer from memory

management overhead. Specifically applications aren't too impressed with the number of 4kB buffers they have to juggle. Zero-copy TCP makes most sense with larger memory transfers so using 16kB or 32kB buffers (with the help of HW-GRO) feels more natural.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

From: Jakub Kicinski <[email protected]>

Unlike most of our APIs the rx-buf-len param does not have an associated max value. In theory user could set this value pretty high, but in practice most NICs have limits due to the width of the length fields in the descriptors.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

From: Jakub Kicinski <[email protected]>

Distinguish between rx_buf_len being driver default vs user config. Use 0 as a special value meaning "unset" or "restore driver default". This will be necessary later on to configure it per-queue, but the ability to restore defaults may be useful in itself.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Quoted 8 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

Distinguish between rx_buf_len being driver default vs user config. Use 0 as a special value meaning "unset" or "restore driver default". This will be necessary later on to configure it per-queue, but the ability to restore defaults may be useful in itself.

Quoted 2 lines

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

I wonder if it should be extended to the other driver using rx_buf_len, hns3. For that, I think the default buf size would be HNS3_DEFAULT_RX_BUF_LEN.

Other than that, seems fine to me,

Reviewed-by Mina Almasry <[email protected]>

Quoted 9 lines · Mina Almasry

Quoted 8 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

Distinguish between rx_buf_len being driver default vs user config. Use 0 as a special value meaning "unset" or "restore driver default". This will be necessary later on to configure it per-queue, but the ability to restore defaults may be useful in itself.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

I wonder if it should be extended to the other driver using rx_buf_len, hns3. For that, I think the default buf size would be HNS3_DEFAULT_RX_BUF_LEN.

I'd rather avoid growing the series even more, let's follow up on that in a separate patch on top, that should be just fine. And thanks for the review

Quoted 3 lines

Other than that, seems fine to me,

Reviewed-by Mina Almasry <[email protected]>

With the said above, do you want me to retain the review tag?

Quoted 22 lines · Pavel Begunkov

Quoted 9 lines · Mina Almasry

Quoted 8 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

Distinguish between rx_buf_len being driver default vs user config. Use 0 as a special value meaning "unset" or "restore driver default". This will be necessary later on to configure it per-queue, but the ability to restore defaults may be useful in itself.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

I wonder if it should be extended to the other driver using rx_buf_len, hns3. For that, I think the default buf size would be HNS3_DEFAULT_RX_BUF_LEN.

I'd rather avoid growing the series even more, let's follow up on that in a separate patch on top, that should be just fine. And thanks for the review

Quoted 6 lines

Quoted 3 lines

Other than that, seems fine to me,

Reviewed-by Mina Almasry <[email protected]>

With the said above, do you want me to retain the review tag?

I initially thought adding my reviewed-by would be fine, but on closer look, doesn't this series break rx_buf_len setting for hns3? AFAICT so far, in patch 3 you're adding a check to ethnl_set_rings where it'll be an error if rx_buf_len > rx_buf_len_max, and i'm guessing if the driver never sets rx_buf_len_max it'll be 0 initialized and that check would always fail? Or did I miss something?

From: Jakub Kicinski <[email protected]>

hds_thresh and hds_config are both inside struct netdev_config but have quite different semantics. hds_config is the user config with ternary semantics (on/off/unset). hds_thresh is a straight up value, populated by the driver at init and only modified by user space. We don't expect the drivers to have to pick a special hds_thresh value based on other configuration.

The two approaches have different advantages and downsides. hds_thresh ("direct value") gives core easy access to current device settings, but there's no way to express whether the value comes from the user. It also requires the initialization by the driver.

hds_config ("user config values") tells us what user wanted, but doesn't give us the current value in the core.

Try to explain this a bit in the comments, so at we make a conscious choice for new values which semantics we expect.

Move the init inside ethtool_ringparam_get_cfg() to reflect the semantics. Commit 216a61d33c07 ("net: ethtool: fix ethtool_ringparam_get_cfg() returns a hds_thresh value always as 0.") added the setting for the benefit of netdevsim which doesn't touch the value at all on get. Again, this is just to clarify the intention, shouldn't cause any functional change.

Signed-off-by Jakub Kicinski <[email protected]>

[pavel: applied clarification on relationship b/w HDS thresh and config]

Signed-off-by Pavel Begunkov <[email protected]>

Quoted 22 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

hds_thresh and hds_config are both inside struct netdev_config but have quite different semantics. hds_config is the user config with ternary semantics (on/off/unset). hds_thresh is a straight up value, populated by the driver at init and only modified by user space. We don't expect the drivers to have to pick a special hds_thresh value based on other configuration.

The two approaches have different advantages and downsides. hds_thresh ("direct value") gives core easy access to current device settings, but there's no way to express whether the value comes from the user. It also requires the initialization by the driver.

hds_config ("user config values") tells us what user wanted, but doesn't give us the current value in the core.

Try to explain this a bit in the comments, so at we make a conscious choice for new values which semantics we expect.

Quoted 7 lines

Move the init inside ethtool_ringparam_get_cfg() to reflect the semantics. Commit 216a61d33c07 ("net: ethtool: fix ethtool_ringparam_get_cfg() returns a hds_thresh value always as 0.") added the setting for the benefit of netdevsim which doesn't touch the value at all on get. Again, this is just to clarify the intention, shouldn't cause any functional change.

TBH I can't say that moving the init to before dev->ethtool_ops->get_ringparam(dev, param, kparam, extack) made me understand semantics better.

If you do a respin, maybe a comment above
the kparam->hds_thresh to say what you mean would help the next reader understand.

From: Jakub Kicinski <[email protected]>

Add rx_buf_len to configuration maintained by the core. Use "three-state" semantics where 0 means "driver default".

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

From: Jakub Kicinski <[email protected]>

Switch from using a constant to storing the BNXT_RX_PAGE_SIZE inside struct bnxt. This will allow configuring the page size at runtime in subsequent patches.

The MSS size calculation for older chip continues to use the constant. I'm intending to support the configuration only on more recent HW, looks like on older chips setting this per queue won't work, and that's the ultimate goal.

This patch should not change the current behavior as value read from the struct will always be BNXT_RX_PAGE_SIZE at this stage.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Quoted 15 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

Switch from using a constant to storing the BNXT_RX_PAGE_SIZE inside struct bnxt. This will allow configuring the page size at runtime in subsequent patches.

The MSS size calculation for older chip continues to use the constant. I'm intending to support the configuration only on more recent HW, looks like on older chips setting this per queue won't work, and that's the ultimate goal.

This patch should not change the current behavior as value read from the struct will always be BNXT_RX_PAGE_SIZE at this stage.

Quoted 2 lines

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Reviewed-by Mina Almasry <[email protected]>

nit: AFAIU BNXT_RX_PAGE_SIZE should be unused after this? You could delete the definition in bnxt.h if so.

From: Jakub Kicinski <[email protected]>

If user decides to increase the buffer size for agg ring we need to ask the page pool for higher order pages. There is no need to use larger pages for header frags, if user increase the size of agg ring buffers switch to separate header page automatically.

Signed-off-by Jakub Kicinski <[email protected]>

[pavel: calculate adjust max_len]

Signed-off-by Pavel Begunkov <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +0 -0

index 5307b33ea1c7..d3d9b72ef313 100644
@@ -3824,11 +3824,13 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
        pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;

Quoted 29 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

If user decides to increase the buffer size for agg ring we need to ask the page pool for higher order pages. There is no need to use larger pages for header frags, if user increase the size of agg ring buffers switch to separate header page automatically.

Signed-off-by Jakub Kicinski <[email protected]>

[pavel: calculate adjust max_len]

Signed-off-by Pavel Begunkov <[email protected]>

---
  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +2 -0

index 5307b33ea1c7..d3d9b72ef313 100644
@@ -3824,11 +3824,13 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
         pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;
         if (BNXT_RX_PAGE_MODE(bp))
                 pp.pool_size += bp->rx_ring_size / rx_size_fac;
+
+       pp.order = get_order(bp->rx_page_size);
         pp.nid = numa_node;
         pp.netdev = bp->dev;
         pp.dev = &bp->pdev->dev;

drivers/net/ethernet/broadcom/bnxt/bnxt.c static int bnxt_alloc_rx_page_pool(struct bnxt *bp, · line 3833

Quoted 3 lines

drivers/net/ethernet/broadcom/bnxt/bnxt.c +1 -1

         pp.dma_dir = bp->rx_dir;
-       pp.max_len = PAGE_SIZE;
+       pp.max_len = PAGE_SIZE << pp.order;

nit: I assume this could be pp.max_len = bp->rx_ring_size; if you wanted, since bnxt is not actually using the full compound page in the case that bp->rx_ring_size is not a power of 2. Though doesn't matter much, either way:

Quoted 30 lines · Mina Almasry

Quoted 29 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

If user decides to increase the buffer size for agg ring we need to ask the page pool for higher order pages. There is no need to use larger pages for header frags, if user increase the size of agg ring buffers switch to separate header page automatically.

Signed-off-by Jakub Kicinski <[email protected]>

[pavel: calculate adjust max_len]

Signed-off-by Pavel Begunkov <[email protected]>

---
  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +2 -0

index 5307b33ea1c7..d3d9b72ef313 100644
@@ -3824,11 +3824,13 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
         pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;
         if (BNXT_RX_PAGE_MODE(bp))
                 pp.pool_size += bp->rx_ring_size / rx_size_fac;
+
+       pp.order = get_order(bp->rx_page_size);
         pp.nid = numa_node;
         pp.netdev = bp->dev;
         pp.dev = &bp->pdev->dev;

Quoted 8 lines

drivers/net/ethernet/broadcom/bnxt/bnxt.c static int bnxt_alloc_rx_page_pool(struct bnxt *bp, · line 3833

Quoted 3 lines

drivers/net/ethernet/broadcom/bnxt/bnxt.c +1 -1

         pp.dma_dir = bp->rx_dir;
-       pp.max_len = PAGE_SIZE;
+       pp.max_len = PAGE_SIZE << pp.order;

nit: I assume this could be pp.max_len = bp->rx_ring_size; if you wanted, since bnxt is not actually using the full compound page in the case that bp->rx_ring_size is not a power of 2. Though doesn't matter much, either way:

Yeah, thought it's cleaner to derive it from order in case sth about rx_page_size changes again and it was already overlooked once, and it's pow2 anyway

--
Pavel Begunkov

From: Jakub Kicinski <[email protected]>

bnxt seems to be able to aggregate data up to 32kB without any issue. The driver is already capable of doing this for systems with higher order pages. While for systems with 4k pages we historically preferred to stick to small buffers because they are easier to allocate, the zero-copy APIs remove the allocation problem. The ZC mem is pre-allocated and fixed size.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 ++-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 21 ++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.h +2 -1

index ac841d02d7ad..56aafae568f8 100644
@@ -758,7 +758,8 @@ struct nqe_cn {
 #define BNXT_RX_PAGE_SHIFT PAGE_SHIFT
 #endif
 
-#define BNXT_RX_PAGE_SIZE (1 << BNXT_RX_PAGE_SHIFT)
+#define BNXT_MAX_RX_PAGE_SIZE  (1 << 15)
+#define BNXT_RX_PAGE_SIZE      (1 << BNXT_RX_PAGE_SHIFT)
 
 #define BNXT_MAX_MTU           9500
 

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +2 -0

index 1b37612b1c01..2e130eeeabe5 100644
@@ -835,6 +835,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
        ering->rx_jumbo_pending = bp->rx_agg_ring_size;
        ering->tx_pending = bp->tx_ring_size;
 
+       kernel_ering->rx_buf_len_max = BNXT_MAX_RX_PAGE_SIZE;
+       kernel_ering->rx_buf_len = bp->rx_page_size;
        kernel_ering->hds_thresh_max = BNXT_HDS_THRESHOLD_MAX;
 }
 

From: Jakub Kicinski <[email protected]>

netdev_config manipulation will become slightly more complicated soon and we will need to call if from ethtool as well as queue API. Encapsulate the logic into helper functions.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 net/core/Makefile        |  2 +-
 net/core/dev.c           |  7 ++-----
 net/core/dev.h           |  5 +++++
 net/core/netdev_config.c | 43 ++++++++++++++++++++++++++++++++++++++++
 net/ethtool/netlink.c    | 14 ++++++-------
 5 files changed, 57 insertions(+), 14 deletions(-)
 create mode 100644 net/core/netdev_config.c

net/core/Makefile +1 -1

index b2a76ce33932..4db487396094 100644
@@ -19,7 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
 
 obj-y += net-sysfs.o
 obj-y += hotdata.o
-obj-y += netdev_rx_queue.o
+obj-y += netdev_config.o netdev_rx_queue.o
 obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o
 obj-$(CONFIG_PROC_FS) += net-procfs.o
 obj-$(CONFIG_NET_PKTGEN) += pktgen.o

net/core/dev.c +2 -5

index 5a3c0f40a93f..7cd4e5eab441 100644
@@ -11873,10 +11873,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
        if (!dev->ethtool)
                goto free_all;
 
-       dev->cfg = kzalloc(sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
-       if (!dev->cfg)
+       if (netdev_alloc_config(dev))
                goto free_all;
-       dev->cfg_pending = dev->cfg;
 
        dev->num_napi_configs = maxqs;
        napi_config_sz = array_size(maxqs, sizeof(*dev->napi_config));

@@ -11947,8 +11945,7 @@ void free_netdev(struct net_device *dev)
                return;
        }
 
-       WARN_ON(dev->cfg != dev->cfg_pending);
-       kfree(dev->cfg);
+       netdev_free_config(dev);
        kfree(dev->ethtool);
        netif_free_tx_queues(dev);
        netif_free_rx_queues(dev);

net/core/dev.h +5 -0

index d6b08d435479..7041c8bd2a0f 100644
@@ -92,6 +92,11 @@ extern struct rw_semaphore dev_addr_sem;
 extern struct list_head net_todo_list;
 void netdev_run_todo(void);
 
+int netdev_alloc_config(struct net_device *dev);
+void __netdev_free_config(struct netdev_config *cfg);
+void netdev_free_config(struct net_device *dev);
+int netdev_reconfig_start(struct net_device *dev);
+
 /* netdev management, shared between various uAPI entry points */
 struct netdev_name_node {
        struct hlist_node hlist;

net/core/netdev_config.c new file +32 -0

new file mode 100644
index 000000000000..270b7f10a192
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/netdevice.h>
+#include <net/netdev_queues.h>
+
+#include "dev.h"
+
+int netdev_alloc_config(struct net_device *dev)
+{
+       struct netdev_config *cfg;
+
+       cfg = kzalloc(sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
+       if (!cfg)
+               return -ENOMEM;
+
+       dev->cfg = cfg;
+       dev->cfg_pending = cfg;
+       return 0;
+}
+
+void __netdev_free_config(struct netdev_config *cfg)
+{
+       kfree(cfg);
+}
+
+void netdev_free_config(struct net_device *dev)
+{
+       WARN_ON(dev->cfg != dev->cfg_pending);
+       __netdev_free_config(dev->cfg);
+}
+
+int netdev_reconfig_start(struct net_device *dev)

From: Jakub Kicinski <[email protected]>

Create an array of config structs to store per-queue config. Pass these structs in the queue API. Drivers can also retrieve the config for a single queue calling netdev_queue_config() directly.

Signed-off-by Jakub Kicinski <[email protected]>

[pavel: patch up mlx callbacks with unused qcfg]

Signed-off-by Pavel Begunkov <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  8 ++-
 drivers/net/ethernet/google/gve/gve_main.c    |  9 ++-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  9 +--
 drivers/net/netdevsim/netdev.c                |  6 +-
 include/net/netdev_queues.h                   | 19 ++++++
 net/core/dev.h                                |  3 +
 net/core/netdev_config.c                      | 58 +++++++++++++++++++
 net/core/netdev_rx_queue.c                    | 11 +++-
 8 files changed, 109 insertions(+), 14 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +6 -2

index d3d9b72ef313..48ff6f024e07 100644
@@ -15824,7 +15824,9 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
        .get_base_stats         = bnxt_get_base_stats,
 };
 
-static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
+static int bnxt_queue_mem_alloc(struct net_device *dev,
+                               struct netdev_queue_config *qcfg,
+                               void *qmem, int idx)
 {
        struct bnxt_rx_ring_info *rxr, *clone;
        struct bnxt *bp = netdev_priv(dev);

@@ -15992,7 +15994,9 @@ static void bnxt_copy_rx_ring(struct bnxt *bp,
        dst->rx_agg_bmap = src->rx_agg_bmap;
 }
 
-static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
+static int bnxt_queue_start(struct net_device *dev,
+                           struct netdev_queue_config *qcfg,
+                           void *qmem, int idx)
 {
        struct bnxt *bp = netdev_priv(dev);
        struct bnxt_rx_ring_info *rxr, *clone;

drivers/net/ethernet/google/gve/gve_main.c +6 -3

index 1f411d7c4373..f40edab616d8 100644
@@ -2580,8 +2580,9 @@ static void gve_rx_queue_mem_free(struct net_device *dev, void *per_q_mem)
                gve_rx_free_ring_dqo(priv, gve_per_q_mem, &cfg);
 }
 
-static int gve_rx_queue_mem_alloc(struct net_device *dev, void *per_q_mem,
-                                 int idx)
+static int gve_rx_queue_mem_alloc(struct net_device *dev,
+                                 struct netdev_queue_config *qcfg,
+                                 void *per_q_mem, int idx)
 {
        struct gve_priv *priv = netdev_priv(dev);
        struct gve_rx_alloc_rings_cfg cfg = {0};

@@ -2602,7 +2603,9 @@ static int gve_rx_queue_mem_alloc(struct net_device *dev, void *per_q_mem,
        return err;
 }
 
-static int gve_rx_queue_start(struct net_device *dev, void *per_q_mem, int idx)
+static int gve_rx_queue_start(struct net_device *dev,
+                             struct netdev_queue_config *qcfg,
+                             void *per_q_mem, int idx)
 {
        struct gve_priv *priv = netdev_priv(dev);
        struct gve_rx_ring *gve_per_q_mem;

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +5 -4

index 21bb88c5d3dc..83264c17a4f7 100644
@@ -5541,8 +5541,9 @@ struct mlx5_qmgmt_data {
        struct mlx5e_channel_param cparam;
 };
 
-static int mlx5e_queue_mem_alloc(struct net_device *dev, void *newq,
-                                int queue_index)
+static int mlx5e_queue_mem_alloc(struct net_device *dev,
+                                struct netdev_queue_config *qcfg,
+                                void *newq, int queue_index)
 {
        struct mlx5_qmgmt_data *new = (struct mlx5_qmgmt_data *)newq;
        struct mlx5e_priv *priv = netdev_priv(dev);

@@ -5603,8 +5604,8 @@ static int mlx5e_queue_stop(struct net_device *dev, void *oldq, int queue_index)
        return 0;
 }
 
-static int mlx5e_queue_start(struct net_device *dev, void *newq,
-                            int queue_index)
+static int mlx5e_queue_start(struct net_device *dev, struct netdev_queue_config *qcfg,
+                            void *newq, int queue_index)
 {
        struct mlx5_qmgmt_data *new = (struct mlx5_qmgmt_data *)newq;
        struct mlx5e_priv *priv = netdev_priv(dev);

drivers/net/netdevsim/netdev.c +4 -2

index 0178219f0db5..985c3403ec57 100644
@@ -733,7 +733,8 @@ struct nsim_queue_mem {
 };
 
 static int
-nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
+nsim_queue_mem_alloc(struct net_device *dev, struct netdev_queue_config *qcfg,
+                    void *per_queue_mem, int idx)
 {
        struct nsim_queue_mem *qmem = per_queue_mem;
        struct netdevsim *ns = netdev_priv(dev);

@@ -782,7 +783,8 @@ static void nsim_queue_mem_free(struct net_device *dev, void *per_queue_mem)
 }
 
 static int
-nsim_queue_start(struct net_device *dev, void *per_queue_mem, int idx)
+nsim_queue_start(struct net_device *dev, struct netdev_queue_config *qcfg,
+                void *per_queue_mem, int idx)
 {
        struct nsim_queue_mem *qmem = per_queue_mem;
        struct netdevsim *ns = netdev_priv(dev);

include/net/netdev_queues.h +0 -0

index d73f9023c96f..b850cff71d12 100644
@@ -32,6 +32,13 @@ struct netdev_config {
        /** @hds_config: HDS enabled (ETHTOOL_A_RINGS_TCP_DATA_SPLIT).

include/net/netdev_queues.h +12 -0

 /* See the netdev.yaml spec for definition of each statistic */

@@ -136,6 +143,10 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  *
  * @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.
  *

@@ -153,12 +164,17 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  */
 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,

@@ -166,6 +182,9 @@ struct netdev_queue_mgmt_ops {
                                  int idx);
 };
 
+void netdev_queue_config(struct net_device *dev, int rxq,
+                        struct netdev_queue_config *qcfg);
+
 /**
  * DOC: Lockless queue stopping / waking helpers.
  *

net/core/dev.h +3 -0

index 7041c8bd2a0f..a553a0f1f846 100644
@@ -9,6 +9,7 @@
 #include <net/netdev_lock.h>
 
 struct net;
+struct netdev_queue_config;
 struct netlink_ext_ack;
 struct cpumask;
 

@@ -96,6 +97,8 @@ int netdev_alloc_config(struct net_device *dev);
 void __netdev_free_config(struct netdev_config *cfg);
 void netdev_free_config(struct net_device *dev);
 int netdev_reconfig_start(struct net_device *dev);
+void __netdev_queue_config(struct net_device *dev, int rxq,
+                          struct netdev_queue_config *qcfg, bool pending);
 
 /* netdev management, shared between various uAPI entry points */
 struct netdev_name_node {

net/core/netdev_config.c +0 -0

index 270b7f10a192..bad2d53522f0 100644

net/core/netdev_config.c +17 -0

+       cfg->qcfg = kcalloc(maxqs, sizeof(*cfg->qcfg), GFP_KERNEL_ACCOUNT);
+       if (!cfg->qcfg)
+               goto err_free_cfg;
+
        dev->cfg = cfg;
        dev->cfg_pending = cfg;
        return 0;
+
+err_free_cfg:
+       kfree(cfg);
+       return -ENOMEM;
 }
 
 void __netdev_free_config(struct netdev_config *cfg)
 {
+       kfree(cfg->qcfg);
        kfree(cfg);
 }
 

@@ -32,12 +43,59 @@ void netdev_free_config(struct net_device *dev)
 int netdev_reconfig_start(struct net_device *dev)
 {
        struct netdev_config *cfg;
+       unsigned int maxqs;
 
        WARN_ON(dev->cfg != dev->cfg_pending);
        cfg = kmemdup(dev->cfg, sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
        if (!cfg)
                return -ENOMEM;
 
+       maxqs = max(dev->num_rx_queues, dev->num_tx_queues);
+       cfg->qcfg = kmemdup_array(dev->cfg->qcfg, maxqs, sizeof(*cfg->qcfg),
+                                 GFP_KERNEL_ACCOUNT);
+       if (!cfg->qcfg)
+               goto err_free_cfg;
+
        dev->cfg_pending = cfg;
        return 0;
+

net/core/netdev_config.c +14 -0

+ * @qcfg is an output parameter and is always fully initialized by this
+ * function. Some values may not be set by the user, drivers may either
+ * deal with the "unset" values in @qcfg, or provide the callback
+ * to populate defaults in queue_management_ops.
+ *
+ * Note that this helper returns pending config, as it is expected that
+ * "old" queues are retained until config is successful so they can
+ * be restored directly without asking for the config.
+ */
+void netdev_queue_config(struct net_device *dev, int rxq,
+                        struct netdev_queue_config *qcfg)
+{
+       __netdev_queue_config(dev, rxq, qcfg, true);
 }
+EXPORT_SYMBOL(netdev_queue_config);

net/core/netdev_rx_queue.c +8 -4

index 3bf1151d8061..fb87ce219a8a 100644
@@ -7,12 +7,14 @@
 #include <net/netdev_rx_queue.h>
 #include <net/page_pool/memory_provider.h>
 
+#include "dev.h"
 #include "page_pool_priv.h"
 
 int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
 {
        struct netdev_rx_queue *rxq = __netif_get_rx_queue(dev, rxq_idx);
        const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
+       struct netdev_queue_config qcfg;
        void *new_mem, *old_mem;
        int err;
 

@@ -32,7 +34,9 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
                goto err_free_new_mem;
        }
 
-       err = qops->ndo_queue_mem_alloc(dev, new_mem, rxq_idx);
+       netdev_queue_config(dev, rxq_idx, &qcfg);
+
+       err = qops->ndo_queue_mem_alloc(dev, &qcfg, new_mem, rxq_idx);
        if (err)
                goto err_free_old_mem;
 

@@ -45,7 +49,7 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
                if (err)
                        goto err_free_new_queue_mem;
 
-               err = qops->ndo_queue_start(dev, new_mem, rxq_idx);
+               err = qops->ndo_queue_start(dev, &qcfg, new_mem, rxq_idx);
                if (err)
                        goto err_start_queue;
        } else {

@@ -60,6 +64,7 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
        return 0;
 
 err_start_queue:
+       __netdev_queue_config(dev, rxq_idx, &qcfg, false);
        /* Restarting the queue with old_mem should be successful as we haven't
         * changed any of the queue configuration, and there is not much we can
         * do to recover from a failure here.

@@ -67,7 +72,7 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
         * WARN if we fail to recover the old rx queue, and at least free
         * old_mem so we don't also leak that.
         */
-       if (qops->ndo_queue_start(dev, old_mem, rxq_idx)) {
+       if (qops->ndo_queue_start(dev, &qcfg, old_mem, rxq_idx)) {
                WARN(1,
                     "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
                     rxq_idx);
--

2.49.0

Quoted 132 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

Create an array of config structs to store per-queue config. Pass these structs in the queue API. Drivers can also retrieve the config for a single queue calling netdev_queue_config() directly.

Signed-off-by Jakub Kicinski <[email protected]>

[pavel: patch up mlx callbacks with unused qcfg]

Signed-off-by Pavel Begunkov <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  8 ++-
 drivers/net/ethernet/google/gve/gve_main.c    |  9 ++-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  9 +--
 drivers/net/netdevsim/netdev.c                |  6 +-
 include/net/netdev_queues.h                   | 19 ++++++
 net/core/dev.h                                |  3 +
 net/core/netdev_config.c                      | 58 +++++++++++++++++++
 net/core/netdev_rx_queue.c                    | 11 +++-
 8 files changed, 109 insertions(+), 14 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +6 -2

index d3d9b72ef313..48ff6f024e07 100644
@@ -15824,7 +15824,9 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
        .get_base_stats         = bnxt_get_base_stats,
 };
 
-static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
+static int bnxt_queue_mem_alloc(struct net_device *dev,
+                               struct netdev_queue_config *qcfg,
+                               void *qmem, int idx)
 {
        struct bnxt_rx_ring_info *rxr, *clone;
        struct bnxt *bp = netdev_priv(dev);

@@ -15992,7 +15994,9 @@ static void bnxt_copy_rx_ring(struct bnxt *bp,
        dst->rx_agg_bmap = src->rx_agg_bmap;
 }
 
-static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
+static int bnxt_queue_start(struct net_device *dev,
+                           struct netdev_queue_config *qcfg,
+                           void *qmem, int idx)
 {
        struct bnxt *bp = netdev_priv(dev);
        struct bnxt_rx_ring_info *rxr, *clone;

drivers/net/ethernet/google/gve/gve_main.c +6 -3

index 1f411d7c4373..f40edab616d8 100644
@@ -2580,8 +2580,9 @@ static void gve_rx_queue_mem_free(struct net_device *dev, void *per_q_mem)
                gve_rx_free_ring_dqo(priv, gve_per_q_mem, &cfg);
 }
 
-static int gve_rx_queue_mem_alloc(struct net_device *dev, void *per_q_mem,
-                                 int idx)
+static int gve_rx_queue_mem_alloc(struct net_device *dev,
+                                 struct netdev_queue_config *qcfg,
+                                 void *per_q_mem, int idx)
 {
        struct gve_priv *priv = netdev_priv(dev);
        struct gve_rx_alloc_rings_cfg cfg = {0};

@@ -2602,7 +2603,9 @@ static int gve_rx_queue_mem_alloc(struct net_device *dev, void *per_q_mem,
        return err;
 }
 
-static int gve_rx_queue_start(struct net_device *dev, void *per_q_mem, int idx)
+static int gve_rx_queue_start(struct net_device *dev,
+                             struct netdev_queue_config *qcfg,
+                             void *per_q_mem, int idx)
 {
        struct gve_priv *priv = netdev_priv(dev);
        struct gve_rx_ring *gve_per_q_mem;

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +5 -4

index 21bb88c5d3dc..83264c17a4f7 100644
@@ -5541,8 +5541,9 @@ struct mlx5_qmgmt_data {
        struct mlx5e_channel_param cparam;
 };
 
-static int mlx5e_queue_mem_alloc(struct net_device *dev, void *newq,
-                                int queue_index)
+static int mlx5e_queue_mem_alloc(struct net_device *dev,
+                                struct netdev_queue_config *qcfg,
+                                void *newq, int queue_index)
 {
        struct mlx5_qmgmt_data *new = (struct mlx5_qmgmt_data *)newq;
        struct mlx5e_priv *priv = netdev_priv(dev);

@@ -5603,8 +5604,8 @@ static int mlx5e_queue_stop(struct net_device *dev, void *oldq, int queue_index)
        return 0;
 }
 
-static int mlx5e_queue_start(struct net_device *dev, void *newq,
-                            int queue_index)
+static int mlx5e_queue_start(struct net_device *dev, struct netdev_queue_config *qcfg,
+                            void *newq, int queue_index)
 {
        struct mlx5_qmgmt_data *new = (struct mlx5_qmgmt_data *)newq;
        struct mlx5e_priv *priv = netdev_priv(dev);

drivers/net/netdevsim/netdev.c +4 -2

index 0178219f0db5..985c3403ec57 100644
@@ -733,7 +733,8 @@ struct nsim_queue_mem {
 };
 
 static int
-nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
+nsim_queue_mem_alloc(struct net_device *dev, struct netdev_queue_config *qcfg,
+                    void *per_queue_mem, int idx)
 {
        struct nsim_queue_mem *qmem = per_queue_mem;
        struct netdevsim *ns = netdev_priv(dev);

@@ -782,7 +783,8 @@ static void nsim_queue_mem_free(struct net_device *dev, void *per_queue_mem)
 }
 
 static int
-nsim_queue_start(struct net_device *dev, void *per_queue_mem, int idx)
+nsim_queue_start(struct net_device *dev, struct netdev_queue_config *qcfg,
+                void *per_queue_mem, int idx)
 {
        struct nsim_queue_mem *qmem = per_queue_mem;
        struct netdevsim *ns = netdev_priv(dev);

include/net/netdev_queues.h +0 -0

index d73f9023c96f..b850cff71d12 100644
@@ -32,6 +32,13 @@ struct netdev_config {
        /** @hds_config: HDS enabled (ETHTOOL_A_RINGS_TCP_DATA_SPLIT).

include/net/netdev_queues.h struct netdev_config { · line 42

Quoted 10 lines

include/net/netdev_queues.h +7 -0

         */
        u8      hds_config;
+
+       /** @qcfg: per-queue configuration */
+       struct netdev_queue_config *qcfg;
+};
+
+/* Same semantics as fields in struct netdev_config */
+struct netdev_queue_config {
 };

I was very confused why this is empty until I looked at patch 18 :-D

Quoted 67 lines

include/net/netdev_queues.h +12 -0

 /* See the netdev.yaml spec for definition of each statistic */

@@ -136,6 +143,10 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  *
  * @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.
  *

@@ -153,12 +164,17 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  */
 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,

@@ -166,6 +182,9 @@ struct netdev_queue_mgmt_ops {
                                  int idx);
 };
 
+void netdev_queue_config(struct net_device *dev, int rxq,
+                        struct netdev_queue_config *qcfg);
+
 /**
  * DOC: Lockless queue stopping / waking helpers.
  *

net/core/dev.h +3 -0

index 7041c8bd2a0f..a553a0f1f846 100644
@@ -9,6 +9,7 @@
 #include <net/netdev_lock.h>
 
 struct net;
+struct netdev_queue_config;
 struct netlink_ext_ack;
 struct cpumask;
 

@@ -96,6 +97,8 @@ int netdev_alloc_config(struct net_device *dev);
 void __netdev_free_config(struct netdev_config *cfg);
 void netdev_free_config(struct net_device *dev);
 int netdev_reconfig_start(struct net_device *dev);
+void __netdev_queue_config(struct net_device *dev, int rxq,
+                          struct netdev_queue_config *qcfg, bool pending);
 
 /* netdev management, shared between various uAPI entry points */
 struct netdev_name_node {

net/core/netdev_config.c +0 -0

index 270b7f10a192..bad2d53522f0 100644

net/core/netdev_config.c line 17

Quoted 10 lines

net/core/netdev_config.c +2 -0

@@ -8,18 +8,29 @@
 int netdev_alloc_config(struct net_device *dev)
 {
        struct netdev_config *cfg;
+       unsigned int maxqs;
 
        cfg = kzalloc(sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
        if (!cfg)
                return -ENOMEM;
 
+       maxqs = max(dev->num_rx_queues, dev->num_tx_queues);

I honestly did not think about tx queues at all for the queue api thus far. The ndos do specify that api applies to rx queues, and maybe the driver only implemented them to assume indeed the calls are to rx queues. Are you intentionally extending the queue api support for tx queues? Or maybe you're allocing configs for the tx queues to be used in some future?

Other places in this patch series uses num_rx_queues directly. Feels like this should do the same.

Quoted 39 lines

net/core/netdev_config.c +17 -0

+       cfg->qcfg = kcalloc(maxqs, sizeof(*cfg->qcfg), GFP_KERNEL_ACCOUNT);
+       if (!cfg->qcfg)
+               goto err_free_cfg;
+
        dev->cfg = cfg;
        dev->cfg_pending = cfg;
        return 0;
+
+err_free_cfg:
+       kfree(cfg);
+       return -ENOMEM;
 }
 
 void __netdev_free_config(struct netdev_config *cfg)
 {
+       kfree(cfg->qcfg);
        kfree(cfg);
 }
 

@@ -32,12 +43,59 @@ void netdev_free_config(struct net_device *dev)
 int netdev_reconfig_start(struct net_device *dev)
 {
        struct netdev_config *cfg;
+       unsigned int maxqs;
 
        WARN_ON(dev->cfg != dev->cfg_pending);
        cfg = kmemdup(dev->cfg, sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
        if (!cfg)
                return -ENOMEM;
 
+       maxqs = max(dev->num_rx_queues, dev->num_tx_queues);
+       cfg->qcfg = kmemdup_array(dev->cfg->qcfg, maxqs, sizeof(*cfg->qcfg),
+                                 GFP_KERNEL_ACCOUNT);
+       if (!cfg->qcfg)
+               goto err_free_cfg;
+
        dev->cfg_pending = cfg;
        return 0;
+

net/core/netdev_config.c void netdev_free_config(struct net_device *dev) · line 71

Quoted 10 lines

net/core/netdev_config.c +10 -0

+err_free_cfg:
+       kfree(cfg);
+       return -ENOMEM;
+}
+
+void __netdev_queue_config(struct net_device *dev, int rxq,
+                          struct netdev_queue_config *qcfg, bool pending)
+{
+       memset(qcfg, 0, sizeof(*qcfg));
+

This memset 0 is wrong for queue configs like hds_thresh where 0 is a value, not 'restore default'.

Either netdev_queue_config needs to have a comment that says 'only values where 0 is restore default is allowed in this struct', or this function needs to handle 0-as-value configs correctly.

But I wonder if the memset(0) is wrong in general. Isn't this helper trying to grab the _current_ configuration? So qcfg should be seeded with appropriate value from dev->qcfgs[rxq]? This function reads like it's trying to get the default configuration, but in a way that doesn't handle hds_thresh style semantics correctly?

From: Jakub Kicinski <[email protected]>

Pass extack to netdev_rx_queue_restart(). Subsequent change will need it.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

From: Jakub Kicinski <[email protected]>

I imagine (tm) that as the number of per-queue configuration options grows some of them may conflict for certain drivers. While the drivers can obviously do all the validation locally doing so is fairly inconvenient as the config is fed to drivers piecemeal via different ops (for different params and NIC-wide vs per-queue).

Add a centralized callback for validating the queue config in queue ops. The callback gets invoked before each queue restart and when ring params are modified.

For NIC-wide changes the callback gets invoked for each active (or active to-be) queue, and additionally with a negative queue index for NIC-wide defaults. The NIC-wide check is needed in case all queues have an override active when NIC-wide setting is changed to an unsupported one. Alternatively we could check the settings when new queues are enabled (in the channel API), but accepting invalid config is a bad idea. Users may expect that resetting a queue override will always work.

The "trick" of passing a negative index is a bit ugly, we may want to revisit if it causes confusion and bugs. Existing drivers don't care about the index so it "just works".

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/net/netdev_queues.h | 12 ++++++++++++
 net/core/dev.h              |  2 ++
 net/core/netdev_config.c    | 20 ++++++++++++++++++++
 net/core/netdev_rx_queue.c  |  6 ++++++
 net/ethtool/rings.c         |  5 +++++
 5 files changed, 45 insertions(+)

include/net/netdev_queues.h +12 -0

index b850cff71d12..d0cc475ec51e 100644
@@ -147,6 +147,14 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  *                     defaults. Queue config structs are passed to this
  *                     helper before the user-requested settings are applied.
  *
+ * @ndo_queue_cfg_validate: (Optional) Check if queue config is supported.
+ *                     Called when configuration affecting a queue may be
+ *                     changing, either due to NIC-wide config, or config
+ *                     scoped to the queue at a specified index.
+ *                     When NIC-wide config is changed the callback will
+ *                     be invoked for all queues, and in addition to that
+ *                     with a negative queue index for the base settings.
+ *
  * @ndo_queue_mem_alloc: Allocate memory for an RX queue at the specified index.
  *                      The new memory is written at the specified address.
  *

@@ -167,6 +175,10 @@ struct netdev_queue_mgmt_ops {
        void    (*ndo_queue_cfg_defaults)(struct net_device *dev,
                                          int idx,
                                          struct netdev_queue_config *qcfg);
+       int     (*ndo_queue_cfg_validate)(struct net_device *dev,
+                                         int idx,
+                                         struct netdev_queue_config *qcfg,
+                                         struct netlink_ext_ack *extack);
        int     (*ndo_queue_mem_alloc)(struct net_device *dev,
                                       struct netdev_queue_config *qcfg,
                                       void *per_queue_mem,

net/core/dev.h +2 -0

index a553a0f1f846..523d50e6f88d 100644
@@ -99,6 +99,8 @@ void netdev_free_config(struct net_device *dev);
 int netdev_reconfig_start(struct net_device *dev);
 void __netdev_queue_config(struct net_device *dev, int rxq,
                           struct netdev_queue_config *qcfg, bool pending);
+int netdev_queue_config_revalidate(struct net_device *dev,
+                                  struct netlink_ext_ack *extack);
 
 /* netdev management, shared between various uAPI entry points */
 struct netdev_name_node {

net/core/netdev_config.c +3 -0

index bad2d53522f0..fc700b77e4eb 100644
@@ -99,3 +99,23 @@ void netdev_queue_config(struct net_device *dev, int rxq,
        __netdev_queue_config(dev, rxq, qcfg, true);
 }
 EXPORT_SYMBOL(netdev_queue_config);
+
+int netdev_queue_config_revalidate(struct net_device *dev,
+                                  struct netlink_ext_ack *extack)

Quoted 91 lines · Pavel Begunkov

From: Jakub Kicinski <[email protected]>

I imagine (tm) that as the number of per-queue configuration options grows some of them may conflict for certain drivers. While the drivers can obviously do all the validation locally doing so is fairly inconvenient as the config is fed to drivers piecemeal via different ops (for different params and NIC-wide vs per-queue).

Add a centralized callback for validating the queue config in queue ops. The callback gets invoked before each queue restart and when ring params are modified.

For NIC-wide changes the callback gets invoked for each active (or active to-be) queue, and additionally with a negative queue index for NIC-wide defaults. The NIC-wide check is needed in case all queues have an override active when NIC-wide setting is changed to an unsupported one. Alternatively we could check the settings when new queues are enabled (in the channel API), but accepting invalid config is a bad idea. Users may expect that resetting a queue override will always work.

The "trick" of passing a negative index is a bit ugly, we may want to revisit if it causes confusion and bugs. Existing drivers don't care about the index so it "just works".

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/net/netdev_queues.h | 12 ++++++++++++
 net/core/dev.h              |  2 ++
 net/core/netdev_config.c    | 20 ++++++++++++++++++++
 net/core/netdev_rx_queue.c  |  6 ++++++
 net/ethtool/rings.c         |  5 +++++
 5 files changed, 45 insertions(+)

include/net/netdev_queues.h +12 -0

index b850cff71d12..d0cc475ec51e 100644
@@ -147,6 +147,14 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  *                     defaults. Queue config structs are passed to this
  *                     helper before the user-requested settings are applied.
  *
+ * @ndo_queue_cfg_validate: (Optional) Check if queue config is supported.
+ *                     Called when configuration affecting a queue may be
+ *                     changing, either due to NIC-wide config, or config
+ *                     scoped to the queue at a specified index.
+ *                     When NIC-wide config is changed the callback will
+ *                     be invoked for all queues, and in addition to that
+ *                     with a negative queue index for the base settings.
+ *
  * @ndo_queue_mem_alloc: Allocate memory for an RX queue at the specified index.
  *                      The new memory is written at the specified address.
  *

@@ -167,6 +175,10 @@ struct netdev_queue_mgmt_ops {
        void    (*ndo_queue_cfg_defaults)(struct net_device *dev,
                                          int idx,
                                          struct netdev_queue_config *qcfg);
+       int     (*ndo_queue_cfg_validate)(struct net_device *dev,
+                                         int idx,
+                                         struct netdev_queue_config *qcfg,
+                                         struct netlink_ext_ack *extack);
        int     (*ndo_queue_mem_alloc)(struct net_device *dev,
                                       struct netdev_queue_config *qcfg,
                                       void *per_queue_mem,

net/core/dev.h +2 -0

index a553a0f1f846..523d50e6f88d 100644
@@ -99,6 +99,8 @@ void netdev_free_config(struct net_device *dev);
 int netdev_reconfig_start(struct net_device *dev);
 void __netdev_queue_config(struct net_device *dev, int rxq,
                           struct netdev_queue_config *qcfg, bool pending);
+int netdev_queue_config_revalidate(struct net_device *dev,
+                                  struct netlink_ext_ack *extack);
 
 /* netdev management, shared between various uAPI entry points */
 struct netdev_name_node {

net/core/netdev_config.c +3 -0

index bad2d53522f0..fc700b77e4eb 100644
@@ -99,3 +99,23 @@ void netdev_queue_config(struct net_device *dev, int rxq,
        __netdev_queue_config(dev, rxq, qcfg, true);
 }
 EXPORT_SYMBOL(netdev_queue_config);
+
+int netdev_queue_config_revalidate(struct net_device *dev,
+                                  struct netlink_ext_ack *extack)

net/core/netdev_config.c void netdev_queue_config(struct net_device *dev, int rxq, · line 114

Quoted 10 lines

net/core/netdev_config.c +10 -0

+{
+       const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
+       struct netdev_queue_config qcfg;
+       int i, err;
+
+       if (!qops || !qops->ndo_queue_cfg_validate)
+               return 0;
+
+       for (i = -1; i < (int)dev->real_num_rx_queues; i++) {
+               netdev_queue_config(dev, i, &qcfg);

This function as written feels very useless tbh. There is no config passed in from the caller, so the function does a netdev_queue_config, which grabs the current-or-default-config (I'm not sure which tbh), and then validates that is applicable. But of course the current or default configs can be applied, right?

I thought there would be a refactor in a future patch that makes this function useful, but I don't see one.

The qcfg being applied needs to be passed in by the caller of this function, no? That would make sense to me (the caller is wondering if this new config is applicable).

From: Jakub Kicinski <[email protected]>

Core provides a centralized callback for validating per-queue settings but the callback is part of the queue management ops. Having the ops conditionally set complicates the parts of the driver which could otherwise lean on the core to feed it the correct settings.

From: Jakub Kicinski <[email protected]>

In normal operation only a subset of queues is configured for zero-copy. Since zero-copy is the main use for larger buffer sizes we need to configure the sizes per queue.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 46 ++++++++++---------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  6 +--
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h |  2 +-
 4 files changed, 30 insertions(+), 25 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +25 -21

index 467e8a0745e1..50f663777843 100644
@@ -900,7 +900,7 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int budget)
 
 static bool bnxt_separate_head_pool(struct bnxt_rx_ring_info *rxr)
 {
-       return rxr->need_head_pool || PAGE_SIZE > rxr->bnapi->bp->rx_page_size;
+       return rxr->need_head_pool || PAGE_SIZE > rxr->rx_page_size;
 }
 
 static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,

@@ -910,9 +910,9 @@ static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,
 {
        struct page *page;
 
-       if (PAGE_SIZE > bp->rx_page_size) {
+       if (PAGE_SIZE > rxr->rx_page_size) {
                page = page_pool_dev_alloc_frag(rxr->page_pool, offset,
-                                               bp->rx_page_size);
+                                               rxr->rx_page_size);
        } else {
                page = page_pool_dev_alloc_pages(rxr->page_pool);
                *offset = 0;

@@ -1150,9 +1150,9 @@ static struct sk_buff *bnxt_rx_multi_page_skb(struct bnxt *bp,
                return NULL;
        }
        dma_addr -= bp->rx_dma_offset;
-       dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr, bp->rx_page_size,
+       dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr, rxr->rx_page_size,
                                bp->rx_dir);
-       skb = napi_build_skb(data_ptr - bp->rx_offset, bp->rx_page_size);
+       skb = napi_build_skb(data_ptr - bp->rx_offset, rxr->rx_page_size);
        if (!skb) {
                page_pool_recycle_direct(rxr->page_pool, page);
                return NULL;

@@ -1184,7 +1184,7 @@ static struct sk_buff *bnxt_rx_page_skb(struct bnxt *bp,
                return NULL;
        }
        dma_addr -= bp->rx_dma_offset;
-       dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr, bp->rx_page_size,
+       dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr, rxr->rx_page_size,
                                bp->rx_dir);
 
        if (unlikely(!payload))

@@ -1198,7 +1198,7 @@ static struct sk_buff *bnxt_rx_page_skb(struct bnxt *bp,
 
        skb_mark_for_recycle(skb);
        off = (void *)data_ptr - page_address(page);
-       skb_add_rx_frag(skb, 0, page, off, len, bp->rx_page_size);
+       skb_add_rx_frag(skb, 0, page, off, len, rxr->rx_page_size);
        memcpy(skb->data - NET_IP_ALIGN, data_ptr - NET_IP_ALIGN,
               payload + NET_IP_ALIGN);
 

@@ -1283,7 +1283,7 @@ static u32 __bnxt_rx_agg_netmems(struct bnxt *bp,
                if (skb) {
                        skb_add_rx_frag_netmem(skb, i, cons_rx_buf->netmem,
                                               cons_rx_buf->offset,
-                                              frag_len, bp->rx_page_size);
+                                              frag_len, rxr->rx_page_size);
                } else {
                        skb_frag_t *frag = &shinfo->frags[i];
 

@@ -1308,7 +1308,7 @@ static u32 __bnxt_rx_agg_netmems(struct bnxt *bp,
                        if (skb) {
                                skb->len -= frag_len;
                                skb->data_len -= frag_len;
-                               skb->truesize -= bp->rx_page_size;
+                               skb->truesize -= rxr->rx_page_size;
                        }
 
                        --shinfo->nr_frags;

@@ -1323,7 +1323,7 @@ static u32 __bnxt_rx_agg_netmems(struct bnxt *bp,
                }
 
                page_pool_dma_sync_netmem_for_cpu(rxr->page_pool, netmem, 0,
-                                                 bp->rx_page_size);
+                                                 rxr->rx_page_size);
 
                total_frag_len += frag_len;
                prod = NEXT_RX_AGG(prod);

@@ -2276,8 +2276,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
                        if (!skb)
                                goto oom_next_rx;
                } else {
-                       skb = bnxt_xdp_build_skb(bp, skb, agg_bufs,
-                                                rxr->page_pool, &xdp);
+                       skb = bnxt_xdp_build_skb(bp, skb, agg_bufs, rxr, &xdp);
                        if (!skb) {
                                /* we should be able to free the old skb here */
                                bnxt_xdp_buff_frags_free(rxr, &xdp);

@@ -3825,7 +3824,7 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
        if (BNXT_RX_PAGE_MODE(bp))
                pp.pool_size += bp->rx_ring_size / rx_size_fac;
 
-       pp.order = get_order(bp->rx_page_size);
+       pp.order = get_order(rxr->rx_page_size);
        pp.nid = numa_node;
        pp.netdev = bp->dev;
        pp.dev = &bp->pdev->dev;

@@ -4318,6 +4317,8 @@ static void bnxt_init_ring_struct(struct bnxt *bp)
                if (!rxr)
                        goto skip_rx;
 
+               rxr->rx_page_size = bp->rx_page_size;
+
                ring = &rxr->rx_ring_struct;
                rmem = &ring->ring_mem;
                rmem->nr_pages = bp->rx_nr_pages;

@@ -4477,7 +4478,7 @@ static void bnxt_init_one_rx_agg_ring_rxbd(struct bnxt *bp,
        ring = &rxr->rx_agg_ring_struct;
        ring->fw_ring_id = INVALID_HW_RING_ID;
        if ((bp->flags & BNXT_FLAG_AGG_RINGS)) {
-               type = ((u32)bp->rx_page_size << RX_BD_LEN_SHIFT) |
+               type = ((u32)rxr->rx_page_size << RX_BD_LEN_SHIFT) |
                        RX_BD_TYPE_RX_AGG_BD | RX_BD_FLAGS_SOP;
 
                bnxt_init_rxbd_pages(ring, type);

@@ -7042,6 +7043,7 @@ static void bnxt_hwrm_ring_grp_free(struct bnxt *bp)
 
 static void bnxt_set_rx_ring_params_p5(struct bnxt *bp, u32 ring_type,
                                       struct hwrm_ring_alloc_input *req,
+                                      struct bnxt_rx_ring_info *rxr,
                                       struct bnxt_ring_struct *ring)
 {
        struct bnxt_ring_grp_info *grp_info = &bp->grp_info[ring->grp_idx];

@@ -7051,7 +7053,7 @@ static void bnxt_set_rx_ring_params_p5(struct bnxt *bp, u32 ring_type,
        if (ring_type == HWRM_RING_ALLOC_AGG) {
                req->ring_type = RING_ALLOC_REQ_RING_TYPE_RX_AGG;
                req->rx_ring_id = cpu_to_le16(grp_info->rx_fw_ring_id);
-               req->rx_buf_size = cpu_to_le16(bp->rx_page_size);
+               req->rx_buf_size = cpu_to_le16(rxr->rx_page_size);
                enables |= RING_ALLOC_REQ_ENABLES_RX_RING_ID_VALID;
        } else {
                req->rx_buf_size = cpu_to_le16(bp->rx_buf_use_size);

@@ -7065,6 +7067,7 @@ static void bnxt_set_rx_ring_params_p5(struct bnxt *bp, u32 ring_type,
 }
 
 static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
+                                   struct bnxt_rx_ring_info *rxr,
                                    struct bnxt_ring_struct *ring,
                                    u32 ring_type, u32 map_index)
 {

@@ -7121,7 +7124,8 @@ static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
                              cpu_to_le32(bp->rx_ring_mask + 1) :
                              cpu_to_le32(bp->rx_agg_ring_mask + 1);
                if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
-                       bnxt_set_rx_ring_params_p5(bp, ring_type, req, ring);
+                       bnxt_set_rx_ring_params_p5(bp, ring_type, req,
+                                                  rxr, ring);
                break;
        case HWRM_RING_ALLOC_CMPL:
                req->ring_type = RING_ALLOC_REQ_RING_TYPE_L2_CMPL;

@@ -7269,7 +7273,7 @@ static int bnxt_hwrm_rx_ring_alloc(struct bnxt *bp,
        u32 map_idx = bnapi->index;
        int rc;
 
-       rc = hwrm_ring_alloc_send_msg(bp, ring, type, map_idx);
+       rc = hwrm_ring_alloc_send_msg(bp, rxr, ring, type, map_idx);
        if (rc)
                return rc;
 

@@ -7289,7 +7293,7 @@ static int bnxt_hwrm_rx_agg_ring_alloc(struct bnxt *bp,
        int rc;
 
        map_idx = grp_idx + bp->rx_nr_rings;
-       rc = hwrm_ring_alloc_send_msg(bp, ring, type, map_idx);
+       rc = hwrm_ring_alloc_send_msg(bp, rxr, ring, type, map_idx);
        if (rc)
                return rc;
 

@@ -7313,7 +7317,7 @@ static int bnxt_hwrm_cp_ring_alloc_p5(struct bnxt *bp,
 
        ring = &cpr->cp_ring_struct;
        ring->handle = BNXT_SET_NQ_HDL(cpr);
-       rc = hwrm_ring_alloc_send_msg(bp, ring, type, map_idx);
+       rc = hwrm_ring_alloc_send_msg(bp, NULL, ring, type, map_idx);
        if (rc)
                return rc;
        bnxt_set_db(bp, &cpr->cp_db, type, map_idx, ring->fw_ring_id);

@@ -7328,7 +7332,7 @@ static int bnxt_hwrm_tx_ring_alloc(struct bnxt *bp,
        const u32 type = HWRM_RING_ALLOC_TX;
        int rc;
 
-       rc = hwrm_ring_alloc_send_msg(bp, ring, type, tx_idx);
+       rc = hwrm_ring_alloc_send_msg(bp, NULL, ring, type, tx_idx);
        if (rc)
                return rc;
        bnxt_set_db(bp, &txr->tx_db, type, tx_idx, ring->fw_ring_id);

@@ -7354,7 +7358,7 @@ static int bnxt_hwrm_ring_alloc(struct bnxt *bp)
 
                vector = bp->irq_tbl[map_idx].vector;
                disable_irq_nosync(vector);
-               rc = hwrm_ring_alloc_send_msg(bp, ring, type, map_idx);
+               rc = hwrm_ring_alloc_send_msg(bp, NULL, ring, type, map_idx);
                if (rc) {
                        enable_irq(vector);
                        goto err_out;

drivers/net/ethernet/broadcom/bnxt/bnxt.h +1 -0

index 56aafae568f8..4f9d4c71c0e2 100644
@@ -1107,6 +1107,7 @@ struct bnxt_rx_ring_info {
 
        unsigned long           *rx_agg_bmap;
        u16                     rx_agg_bmap_size;
+       u16                     rx_page_size;
        bool                    need_head_pool;
 
        dma_addr_t              rx_desc_mapping[MAX_RX_PAGES];

drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c +3 -3

index 41d3ba56ba41..19dda0201c69 100644
@@ -183,7 +183,7 @@ void bnxt_xdp_buff_init(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
                        u16 cons, u8 *data_ptr, unsigned int len,
                        struct xdp_buff *xdp)
 {
-       u32 buflen = bp->rx_page_size;
+       u32 buflen = rxr->rx_page_size;
        struct bnxt_sw_rx_bd *rx_buf;
        struct pci_dev *pdev;
        dma_addr_t mapping;

@@ -461,7 +461,7 @@ int bnxt_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 
 struct sk_buff *
 bnxt_xdp_build_skb(struct bnxt *bp, struct sk_buff *skb, u8 num_frags,
-                  struct page_pool *pool, struct xdp_buff *xdp)
+                  struct bnxt_rx_ring_info *rxr, struct xdp_buff *xdp)
 {
        struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 

@@ -470,7 +470,7 @@ bnxt_xdp_build_skb(struct bnxt *bp, struct sk_buff *skb, u8 num_frags,
 
        xdp_update_skb_shared_info(skb, num_frags,
                                   sinfo->xdp_frags_size,
-                                  bp->rx_page_size * num_frags,
+                                  rxr->rx_page_size * num_frags,
                                   xdp_buff_is_frag_pfmemalloc(xdp));
        return skb;
 }

drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h +1 -2

index 220285e190fc..8933a0dec09a 100644
@@ -32,6 +32,6 @@ void bnxt_xdp_buff_init(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
 void bnxt_xdp_buff_frags_free(struct bnxt_rx_ring_info *rxr,
                              struct xdp_buff *xdp);
 struct sk_buff *bnxt_xdp_build_skb(struct bnxt *bp, struct sk_buff *skb,
-                                  u8 num_frags, struct page_pool *pool,
+                                  u8 num_frags, struct bnxt_rx_ring_info *rxr,
                                   struct xdp_buff *xdp);
 #endif
--

2.49.0