Re: [RFC v1 13/22] net: add queue config validation callback

[Mina Almasry <almasrymina@google.com>]

On Mon, Jul 28, 2025 at 4:03 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> From: Jakub Kicinski <kuba@kernel.org>
>
> 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 <kuba@kernel.org>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  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(+)
>
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index e3e7ecf91bac..f75313fc78ba 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -146,6 +146,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.
>   *
> @@ -166,6 +174,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,
> diff --git a/net/core/dev.h b/net/core/dev.h
> index 6d7f5e920018..e0d433fb6325 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -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 {
> diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
> index bad2d53522f0..fc700b77e4eb 100644
> --- a/net/core/netdev_config.c
> +++ b/net/core/netdev_config.c
> @@ -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)
> +{
> +       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;
> +

Shouldn't this be like return -EOPNOSUPP; or something? Otherwise how
do you protect drivers (GVE) that support queue API but don't support
a configuring a particular netdev_queue_config from core assuming that
the configuration took place?

> +       for (i = -1; i < (int)dev->real_num_rx_queues; i++) {
> +               netdev_queue_config(dev, i, &qcfg);
> +               err = qops->ndo_queue_cfg_validate(dev, i, &qcfg, extack);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> index b0523eb44e10..7c691eb1a48b 100644
> --- a/net/core/netdev_rx_queue.c
> +++ b/net/core/netdev_rx_queue.c
> @@ -37,6 +37,12 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx,
>
>         netdev_queue_config(dev, rxq_idx, &qcfg);
>
> +       if (qops->ndo_queue_cfg_validate) {
> +               err = qops->ndo_queue_cfg_validate(dev, rxq_idx, &qcfg, extack);
> +               if (err)
> +                       goto err_free_old_mem;
> +       }
> +
>         err = qops->ndo_queue_mem_alloc(dev, &qcfg, new_mem, rxq_idx);
>         if (err)
>                 goto err_free_old_mem;
> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
> index 6a74e7e4064e..7884d10c090f 100644
> --- a/net/ethtool/rings.c
> +++ b/net/ethtool/rings.c
> @@ -4,6 +4,7 @@
>
>  #include "netlink.h"
>  #include "common.h"
> +#include "../core/dev.h"
>
>  struct rings_req_info {
>         struct ethnl_req_info           base;
> @@ -307,6 +308,10 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
>         dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
>         dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
>
> +       ret = netdev_queue_config_revalidate(dev, info->extack);
> +       if (ret)
> +               return ret;
> +
>         ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>                                               &kernel_ringparam, info->extack);
>         return ret < 0 ? ret : 1;
> --
> 2.49.0
>


-- 
Thanks,
Mina