Message-ID: <f566a9c4-af28-4135-bae3-01ea5ad1ba97@redhat.com>
Date: Sun, 7 Sep 2025 13:10:24 +0300
From: mohammad heib <mheib@...hat.com>
To: Simon Horman <horms@...nel.org>, Jacob Keller <jacob.e.keller@...el.com>
Cc: 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 9/5/25 2:46 PM, Simon Horman wrote:
> 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.
Me too :L)
> 
> 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;
>>>   		}
>>>   	}
>>
> 
> 
> 

Thank you, Jacob, Simon, and Aleksandr, for reviewing this. I really 
appreciate your time.

Powered by blists - more mailing lists