Message-ID: <efb80605-187f-4b80-8ba9-8065d1b9e9d0@intel.com>
Date: Wed, 3 Sep 2025 15:25:40 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: <mheib@...hat.com>, <intel-wired-lan@...ts.osuosl.org>
CC: <przemyslawx.patynowski@...el.com>, <jiri@...nulli.us>,
	<netdev@...r.kernel.org>, <horms@...nel.org>,
	<aleksandr.loktionov@...el.com>, <anthony.l.nguyen@...el.com>,
	<przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net-next,v3,2/2] i40e: support generic devlink param
 "max_mac_per_vf"



On 9/3/2025 2:43 PM, mheib@...hat.com wrote:
> From: Mohammad Heib <mheib@...hat.com>
> 
> Currently the i40e driver enforces its own internally calculated per-VF MAC
> filter limit, derived from the number of allocated VFs and available
> hardware resources. This limit is not configurable by the administrator,
> which makes it difficult to control how many MAC addresses each VF may
> use.
> 
> This patch adds support for the new generic devlink runtime parameter
> "max_mac_per_vf" which provides administrators with a way to cap the
> number of MAC addresses a VF can use:
> 
> - When the parameter is set to 0 (default), the driver continues to use
>   its internally calculated limit.
> 
> - When set to a non-zero value, the driver applies this value as a strict
>   cap for VFs, overriding the internal calculation.
> 
> Important notes:
> 
> - The configured value is a theoretical maximum. Hardware limits may
>   still prevent additional MAC addresses from being added, even if the
>   parameter allows it.
> 
> - Since MAC filters are a shared hardware resource across all VFs,
>   setting a high value may cause resource contention and starve other
>   VFs.
> 
> - This change gives administrators predictable and flexible control over
>   VF resource allocation, while still respecting hardware limitations.
> 
> - Previous discussion about this change:
>   https://lore.kernel.org/netdev/20250805134042.2604897-2-dhill@redhat.com
>   https://lore.kernel.org/netdev/20250823094952.182181-1-mheib@redhat.com
> 
> Signed-off-by: Mohammad Heib <mheib@...hat.com>
> ---

This version looks good to me. With or without minor nits relating to
rate limiting and adding mac_add_max to the untrusted message:

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 081a4526a2f0..6e154a8aa474 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -2935,33 +2935,48 @@ 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 for both trusted and
> +	 *   untrusted VFs.
> +	 *   Note:
> +	 *    even when overridden, this is a theoretical maximum; hardware
> +	 *    may reject additional MACs if the absolute HW limit is reached.
>  	 */

Good. I think this is better and allows users to also increase limit for
untrusted VFs without requiring them to become fully "trusted" with the
all-or-nothing approach. Its more flexible in that regard, and avoids
the confusion of the parameter not working because a VF is untrusted.

>  	if (!vf_trusted)
>  		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>  	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;
> +

Nice, a clean way to edit the maximum without needing too much special
casing.

>  	/* 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
>  	 * a to-be-removed state. Account for that.
>  	 */
>  	if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
>  	    (i40e_count_all_filters(vsi) + mac_add_cnt) > 2 * mac_add_max) {
> +		if (pf->max_mac_per_vf == mac_add_max && mac_add_max > 0) {
> +			dev_err(&pf->pdev->dev,
> +				"Cannot add more MAC addresses: VF reached its maximum allowed limit (%d)\n",
> +				mac_add_max);
> +				return -EPERM;
> +		}

Good, having the specific error message will aid system administrators
in debugging.

One thought I had, which isn't a knock on your code as we did the same
before.. should these be rate limited to prevent VF spamming MAC filter
adds clogging up the dmesg buffer?

Given that we didn't do it before, I think its reasonable to not hold
this patch up for such a cleanup.

>  		if (!vf_trusted) {
>  			dev_err(&pf->pdev->dev,
>  				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
>  			return -EPERM;
>  		} else {

We didn't rate limit it before. I am not sure how fast the VF can
actually send messages, so I'm not sure if that change would be required.

You could optionally also report the mac_add_max for the untrusted
message as well, but I think its fine to leave as-is in that case as well.

>  			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;
>  		}
>  	}



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)

Powered by blists - more mailing lists