Message-ID: <zylu4kqwjx4rjatlsggo2v4lspxbkx4efb5mx7tvudhmfjsipz@vf74rx5bpr3l>
Date: Thu, 28 Aug 2025 15:04:50 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: mohammad heib <mheib@...hat.com>
Cc: intel-wired-lan@...ts.osuosl.org, przemyslawx.patynowski@...el.com, 
	netdev@...r.kernel.org, horms@...nel.org, jacob.e.keller@...el.com, 
	aleksandr.loktionov@...el.com, anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com
Subject: Re: [PATCH net v2] i40e: add devlink param to control VF MAC address
 limit

Thu, Aug 28, 2025 at 02:49:26PM +0200, mheib@...hat.com wrote:
>Hi Jiri
>
>On 8/28/25 2:43 PM, Jiri Pirko wrote:
>> Sat, Aug 23, 2025 at 11:49:52AM +0200, mheib@...hat.com wrote:
>> > From: Mohammad Heib <mheib@...hat.com>
>> > 
>> > This patch introduces a new devlink runtime parameter that controls
>> > the maximum number of MAC filters allowed per VF.
>> > 
>> > The parameter is an integer value. If set to a non-zero number, it is
>> > used as a strict per-VF cap. If left at zero, the driver falls back to
>> > the default limit calculated from the number of allocated VFs and
>> > ports.
>> > 
>> > This makes the limit policy explicit and configurable by user space,
>> > instead of being only driver internal logic.
>> > 
>> > Example command to enable per-vf mac limit:
>> > - devlink dev param set pci/0000:3b:00.0 name max_mac_per_vf \
>> > 	value 12 \
>> > 	cmode runtime
>> > 
>> > - Previous discussion about this change:
>> >   https://lore.kernel.org/netdev/20250805134042.2604897-1-dhill@redhat.com
>> > 
>> > Fixes: cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every trusted VF")
>> > Signed-off-by: Mohammad Heib <mheib@...hat.com>
>> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> > ---
>> > Documentation/networking/devlink/i40e.rst     | 22 ++++++++
>> > drivers/net/ethernet/intel/i40e/i40e.h        |  4 ++
>> > .../net/ethernet/intel/i40e/i40e_devlink.c    | 56 ++++++++++++++++++-
>> > .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 25 +++++----
>> > 4 files changed, 95 insertions(+), 12 deletions(-)
>> > 
>> > diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
>> > index d3cb5bb5197e..f8d5b00bb51d 100644
>> > --- a/Documentation/networking/devlink/i40e.rst
>> > +++ b/Documentation/networking/devlink/i40e.rst
>> > @@ -7,6 +7,28 @@ i40e devlink support
>> > This document describes the devlink features implemented by the ``i40e``
>> > device driver.
>> > 
>> > +Parameters
>> > +==========
>> > +
>> > +.. list-table:: Driver specific parameters implemented
>> > +    :widths: 5 5 90
>> > +
>> > +    * - Name
>> > +      - Mode
>> > +      - Description
>> > +    * - ``max_mac_per_vf``
>> > +      - runtime
>> > +      - Controls the maximum number of MAC addresses a VF can use on i40e devices.
>> > +
>> > +        By default (``0``), the driver enforces its internally calculated per-VF
>> > +        MAC filter limit, which is based on the number of allocated VFS.
>> > +
>> > +        If set to a non-zero value, this parameter acts as a strict cap:
>> > +        the driver enforces the maximum of the user-provided value and ignore
>> > +        internally calculated limit.
>> > +
>> > +        The default value is ``0``.
>> > +
>> > Info versions
>> > =============
>> > 
>> > diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
>> > index 801a57a925da..d2d03db2acec 100644
>> > --- a/drivers/net/ethernet/intel/i40e/i40e.h
>> > +++ b/drivers/net/ethernet/intel/i40e/i40e.h
>> > @@ -574,6 +574,10 @@ struct i40e_pf {
>> > 	struct i40e_vf *vf;
>> > 	int num_alloc_vfs;	/* actual number of VFs allocated */
>> > 	u32 vf_aq_requests;
>> > +	/* If set to non-zero, the device uses this value
>> > +	 * as maximum number of MAC filters per VF.
>> > +	 */
>> > +	u32 max_mac_per_vf;
>> > 	u32 arq_overflows;	/* Not fatal, possibly indicative of problems */
>> > 	struct ratelimit_state mdd_message_rate_limit;
>> > 	/* DCBx/DCBNL capability for PF that indicates
>> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
>> > index cc4e9e2addb7..8532e40b5c7d 100644
>> > --- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
>> > +++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
>> > @@ -5,6 +5,42 @@
>> > #include "i40e.h"
>> > #include "i40e_devlink.h"
>> > 
>> > +static int i40e_max_mac_per_vf_set(struct devlink *devlink,
>> > +				   u32 id,
>> > +				   struct devlink_param_gset_ctx *ctx,
>> > +				   struct netlink_ext_ack *extack)
>> > +{
>> > +	struct i40e_pf *pf = devlink_priv(devlink);
>> > +
>> > +	pf->max_mac_per_vf = ctx->val.vu32;
>> > +	return 0;
>> > +}
>> > +
>> > +static int i40e_max_mac_per_vf_get(struct devlink *devlink,
>> > +				   u32 id,
>> > +				   struct devlink_param_gset_ctx *ctx)
>> > +{
>> > +	struct i40e_pf *pf = devlink_priv(devlink);
>> > +
>> > +	ctx->val.vu32 = pf->max_mac_per_vf;
>> > +	return 0;
>> > +}
>> > +
>> > +enum i40e_dl_param_id {
>> > +	I40E_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>> > +	I40E_DEVLINK_PARAM_ID_MAX_MAC_PER_VF,
>> What's so i40 specific about this? Sounds pretty generic to be.
>> 
>> 
>> 
>> > +};
>> > +
>> > +static const struct devlink_param i40e_dl_params[] = {
>> > +	DEVLINK_PARAM_DRIVER(I40E_DEVLINK_PARAM_ID_MAX_MAC_PER_VF,
>> > +			     "max_mac_per_vf",
>> > +			     DEVLINK_PARAM_TYPE_U32,
>> > +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>> > +			     i40e_max_mac_per_vf_get,
>> > +			     i40e_max_mac_per_vf_set,
>> > +			     NULL),
>> > +};
>> > +
>> > static void i40e_info_get_dsn(struct i40e_pf *pf, char *buf, size_t len)
>> > {
>> > 	u8 dsn[8];
>> > @@ -165,7 +201,19 @@ void i40e_free_pf(struct i40e_pf *pf)
>> >   **/
>> > void i40e_devlink_register(struct i40e_pf *pf)
>> > {
>> > -	devlink_register(priv_to_devlink(pf));
>> > +	int err;
>> > +	struct devlink *dl = priv_to_devlink(pf);
>> > +	struct device *dev = &pf->pdev->dev;
>> > +
>> > +	err = devlink_params_register(dl, i40e_dl_params,
>> > +				      ARRAY_SIZE(i40e_dl_params));
>> > +	if (err) {
>> > +		dev_err(dev,
>> > +			"devlink params register failed with error %d", err);
>> > +	}
>> > +
>> > +	devlink_register(dl);
>> > +
>> > }
>> > 
>> > /**
>> > @@ -176,7 +224,11 @@ void i40e_devlink_register(struct i40e_pf *pf)
>> >   **/
>> > void i40e_devlink_unregister(struct i40e_pf *pf)
>> > {
>> > -	devlink_unregister(priv_to_devlink(pf));
>> > +	struct devlink *dl = priv_to_devlink(pf);
>> > +
>> > +	devlink_unregister(dl);
>> > +	devlink_params_unregister(dl, i40e_dl_params,
>> > +				  ARRAY_SIZE(i40e_dl_params));
>> > }
>> > 
>> > /**
>> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> > index 081a4526a2f0..e7c0c791eed1 100644
>> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> > @@ -2935,19 +2935,23 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
>> > 		if (!f)
>> > 			++mac_add_cnt;
>> > 	}
>> > -
>> > -	/* If this VF is not privileged, then we can't add more than a limited
>> > -	 * number of addresses.
>> > +	/* Determine the maximum number of MAC addresses this VF may use.
>> > +	 *
>> > +	 * - For untrusted VFs: use a fixed small limit.
>> > +	 *
>> > +	 * - For trusted VFs: limit is calculated by dividing total MAC
>> > +	 *  filter pool across all VFs/ports.
>> > 	 *
>> > -	 * If this VF is trusted, it can use more resources than untrusted.
>> > -	 * However to ensure that every trusted VF has appropriate number of
>> > -	 * resources, divide whole pool of resources per port and then across
>> > -	 * all VFs.
>> > +	 * - User can override this by devlink param "max_mac_per_vf".
>> > +	 *   If set its value is used as a strict cap.
>> > 	 */
>> > -	if (!vf_trusted)
>> > +	if (!vf_trusted) {
>> > 		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>> > -	else
>> > +	} else {
>> > 		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);
>> > +		if (pf->max_mac_per_vf > 0)
>> > +			mac_add_max = pf->max_mac_per_vf;
>> > +	}
>> > 
>> > 	/* VF can replace all its filters in one step, in this case mac_add_max
>> > 	 * will be added as active and another mac_add_max will be in
>> > @@ -2961,7 +2965,8 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
>> > 			return -EPERM;
>> > 		} else {
>> > 			dev_err(&pf->pdev->dev,
>> > -				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
>> > +				"Cannot add more MAC addresses: trusted VF reached its maximum allowed limit (%d)\n",
>> > +				mac_add_max);
>> > 			return -EPERM;
>> > 		}
>> > 	}
>> > -- 
>> > 2.47.3
>> > 
>If i understand correctly, you’re asking whether this parameter could be
>added as a generic devlink parameter
>rather than a driver-specific one?
>
>
>if that the case, Yes, it could be made generic, but I initially implemented
>it as driver specific because I was targeting i40e only,
>and I thought a generic approach might not be acceptable at this stage
>
>
>I do plan to extend this to other drivers, as mentioned in the v1 patch here:
> - https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20250821233930.127420-1-mheib@redhat.com/
>For now, i'm pushing hard to get this patch into i40e since it affects a
>customer. Once it's accepted i can extend it
>to other drivers and convert it to the generic devlink parameter.

No, please do it as generic from start. Thanks!

>
>
>Thanks,
>

Powered by blists - more mailing lists