Message-ID: <20250905114642.GA551420@horms.kernel.org>
Date: Fri, 5 Sep 2025 12:46:42 +0100
From: Simon Horman <horms@...nel.org>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: mheib@...hat.com, intel-wired-lan@...ts.osuosl.org,
	przemyslawx.patynowski@...el.com, jiri@...nulli.us,
	netdev@...r.kernel.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 Wed, Sep 03, 2025 at 03:25:40PM -0700, Jacob Keller wrote:
> 
> 
> 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>

Thanks, I'm very pleased to see this one coming together.

Reviewed-by: Simon Horman <horms@...nel.org>

> > 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.

+1

> >  	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.

Also, +1.

> 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.

I'm not sure either. I'm more used to rate limits in the datapath,
where network traffic can result in a log.

I think that if we want to go down the path you suggest then we should
look at what other logs fall into the same category: generated by VM admin
actions. And perhaps start by looking in the i40e driver for such cases.

Just my 2c worth on this one.

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

Powered by blists - more mailing lists