| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 29 Sep 2023 11:23:49 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: <edward.cree@....com>, <linux-net-drivers@....com>, <davem@...emloft.net>,
<kuba@...nel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>
CC: Edward Cree <ecree.xilinx@...il.com>, <netdev@...r.kernel.org>,
<habetsm.xilinx@...il.com>, <sudheer.mogilappagari@...el.com>,
<jdamato@...tly.com>, <andrew@...n.ch>, <mw@...ihalf.com>,
<linux@...linux.org.uk>, <sgoutham@...vell.com>, <gakula@...vell.com>,
<sbhatta@...vell.com>, <hkelam@...vell.com>, <saeedm@...dia.com>,
<leon@...nel.org>
Subject: Re: [PATCH v4 net-next 4/7] net: ethtool: let the core choose RSS
context IDs
On 9/27/2023 11:13 AM, edward.cree@....com wrote:
> From: Edward Cree <ecree.xilinx@...il.com>
>
> Add a new API to create/modify/remove RSS contexts, that passes in the
> newly-chosen context ID (not as a pointer) rather than leaving the
> driver to choose it on create. Also pass in the ctx, allowing drivers
> to easily use its private data area to store their hardware-specific
> state.
> Keep the existing .set_rxfh_context API for now as a fallback, but
> deprecate it.
>
> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
> include/linux/ethtool.h | 40 ++++++++++++++++++++++++---
> net/core/dev.c | 15 ++++++++---
> net/ethtool/ioctl.c | 60 ++++++++++++++++++++++++++++++-----------
> 3 files changed, 93 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 229a23571008..975fda7218f8 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -747,10 +747,33 @@ struct ethtool_mm_stats {
> * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
> * hash key, and/or hash function assiciated to the given rss context.
> * Returns a negative error code or zero.
> - * @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting
> + * @create_rxfh_context: Create a new RSS context with the specified RX flow
> + * hash indirection table, hash key, and hash function.
> + * Arguments which are set to %NULL or zero will be populated to
> + * appropriate defaults by the driver.
> + * The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + * note that the indir table, hkey and hfunc are not yet populated as
> + * of this call. The driver does not need to update these; the core
> + * will do so if this op succeeds.
> + * If the driver provides this method, it must also provide
> + * @modify_rxfh_context and @remove_rxfh_context.
> + * Returns a negative error code or zero.
> + * @modify_rxfh_context: Reconfigure the specified RSS context. Allows setting
> * the contents of the RX flow hash indirection table, hash key, and/or
> - * hash function associated to the given context. Arguments which are set
> - * to %NULL or zero will remain unchanged.
> + * hash function associated with the given context.
> + * Arguments which are set to %NULL or zero will remain unchanged.
> + * The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + * note that it will still contain the *old* settings. The driver does
> + * not need to update these; the core will do so if this op succeeds.
> + * Returns a negative error code or zero. An error code must be returned
> + * if at least one unsupported change was requested.
> + * @remove_rxfh_context: Remove the specified RSS context.
> + * The &struct ethtool_rxfh_context for this context is passed in @ctx.
> + * Returns a negative error code or zero.
> + * @set_rxfh_context: Deprecated API to create, remove and configure RSS
> + * contexts. Allows setting the contents of the RX flow hash indirection
> + * table, hash key, and/or hash function associated to the given context.
> + * Arguments which are set to %NULL or zero will remain unchanged.
> * Returns a negative error code or zero. An error code must be returned
> * if at least one unsupported change was requested.
> * @get_channels: Get number of channels.
> @@ -901,6 +924,17 @@ struct ethtool_ops {
> const u8 *key, const u8 hfunc);
> int (*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
> u8 *hfunc, u32 rss_context);
> + int (*create_rxfh_context)(struct net_device *,
> + struct ethtool_rxfh_context *ctx,
> + const u32 *indir, const u8 *key,
> + const u8 hfunc, u32 rss_context);
> + int (*modify_rxfh_context)(struct net_device *,
> + struct ethtool_rxfh_context *ctx,
> + const u32 *indir, const u8 *key,
> + const u8 hfunc, u32 rss_context);
> + int (*remove_rxfh_context)(struct net_device *,
> + struct ethtool_rxfh_context *ctx,
> + u32 rss_context);
> int (*set_rxfh_context)(struct net_device *, const u32 *indir,
> const u8 *key, const u8 hfunc,
> u32 *rss_context, bool delete);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 05e95abdfd17..637218adca22 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10882,16 +10882,23 @@ static void netdev_rss_contexts_free(struct net_device *dev)
> struct ethtool_rxfh_context *ctx;
> unsigned long context;
>
> - if (dev->ethtool_ops->set_rxfh_context)
> + if (dev->ethtool_ops->create_rxfh_context ||
> + dev->ethtool_ops->set_rxfh_context)
> xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
> u32 *indir = ethtool_rxfh_context_indir(ctx);
> u8 *key = ethtool_rxfh_context_key(ctx);
> u32 concast = context;
>
> xa_erase(&dev->ethtool->rss_ctx, context);
> - dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> - ctx->hfunc, &concast,
> - true);
> + if (dev->ethtool_ops->create_rxfh_context)
> + dev->ethtool_ops->remove_rxfh_context(dev, ctx,
> + context);
A bit weird here, but the idea is that you must support both create,
remove, and modify, so we only check the presence of "create". Ok.
> + else
> + dev->ethtool_ops->set_rxfh_context(dev, indir,
> + key,
> + ctx->hfunc,
> + &concast,
> + true);
> kfree(ctx);
> }
> xa_destroy(&dev->ethtool->rss_ctx);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 1d13bc8fbb75..c23d2bd3cd2a 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
> return -EINVAL;
> /* Most drivers don't handle rss_context, check it's 0 as well */
> - if (rxfh.rss_context && !ops->set_rxfh_context)
> + if (rxfh.rss_context && !(ops->create_rxfh_context ||
> + ops->set_rxfh_context))
> return -EOPNOTSUPP;
> create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>
> @@ -1349,8 +1350,24 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> }
> ctx->indir_size = dev_indir_size;
> ctx->key_size = dev_key_size;
> - ctx->hfunc = rxfh.hfunc;
> ctx->priv_size = ops->rxfh_priv_size;
> + /* Initialise to an empty context */
> + ctx->indir_no_change = ctx->key_no_change = 1;
> + ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
> + if (ops->create_rxfh_context) {
> + u32 limit = dev->ethtool->rss_ctx_max_id ?: U32_MAX;
> + u32 ctx_id;
> +
> + /* driver uses new API, core allocates ID */
> + ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
> + XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
> + if (ret < 0) {
> + kfree(ctx);
> + goto out;
> + }
> + WARN_ON(!ctx_id); /* can't happen */
> + rxfh.rss_context = ctx_id;
> + }
> } else if (rxfh.rss_context) {
> ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
> if (!ctx) {
> @@ -1359,15 +1376,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> }
> }
>
> - if (rxfh.rss_context)
> - ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
> - &rxfh.rss_context, delete);
> - else
> + if (rxfh.rss_context) {
> + if (ops->create_rxfh_context) {
> + if (create)
> + ret = ops->create_rxfh_context(dev, ctx, indir,
> + hkey, rxfh.hfunc,
> + rxfh.rss_context);
> + else if (delete)
> + ret = ops->remove_rxfh_context(dev, ctx,
> + rxfh.rss_context);
> + else
> + ret = ops->modify_rxfh_context(dev, ctx, indir,
> + hkey, rxfh.hfunc,
> + rxfh.rss_context);
> + } else {
> + ret = ops->set_rxfh_context(dev, indir, hkey,
> + rxfh.hfunc,
> + &rxfh.rss_context, delete);
> + }
> + } else {
> ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> + }
> if (ret) {
> - if (create)
> + if (create) {
> /* failed to create, free our new tracking entry */
> + if (ops->create_rxfh_context)
> + xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context);
> kfree(ctx);
> + }
> goto out;
> }
>
> @@ -1383,12 +1419,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> dev->priv_flags |= IFF_RXFH_CONFIGURED;
> }
> /* Update rss_ctx tracking */
> - if (create) {
> - /* Ideally this should happen before calling the driver,
> - * so that we can fail more cleanly; but we don't have the
> - * context ID until the driver picks it, so we have to
> - * wait until after.
> - */
> + if (create && !ops->create_rxfh_context) {
> + /* driver uses old API, it chose context ID */
> if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
> /* context ID reused, our tracking is screwed */
> kfree(ctx);
> @@ -1400,8 +1432,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> kfree(ctx);
> goto out;
> }
> - ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> - ctx->key_no_change = !rxfh.key_size;
> }
> if (delete) {
> WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
>
Powered by blists - more mailing lists