Message-ID: <3280699a-7cd3-407f-8875-8186de967d15@redhat.com>
Date: Wed, 3 Sep 2025 22:05:26 +0300
From: mohammad heib <mheib@...hat.com>
To: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>,
 "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
Cc: "przemyslawx.patynowski@...el.com" <przemyslawx.patynowski@...el.com>,
 "jiri@...nulli.us" <jiri@...nulli.us>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "horms@...nel.org" <horms@...nel.org>,
 "Keller, Jacob E" <jacob.e.keller@...el.com>,
 "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
 "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net-next,2/2] i40e: support generic devlink param
 "max_mac_per_vf"

Hi Aleksandr,

Thanks again for your review.
I’ve updated the documentation and commit message in v2 to address your 
feedback.

Appreciate your time!
On 9/3/25 3:35 PM, Loktionov, Aleksandr wrote:
> *From:*mohammad heib <mheib@...hat.com>
> *Sent:* Wednesday, September 3, 2025 12:01 PM
> *To:* Loktionov, Aleksandr <aleksandr.loktionov@...el.com>; intel-wired- 
> lan@...ts.osuosl.org
> *Cc:* przemyslawx.patynowski@...el.com; jiri@...nulli.us; 
> netdev@...r.kernel.org; horms@...nel.org; Keller, Jacob E 
> <jacob.e.keller@...el.com>; Nguyen, Anthony L 
> <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw 
> <przemyslaw.kitszel@...el.com>
> *Subject:* Re: [PATCH net-next,2/2] i40e: support generic devlink param 
> "max_mac_per_vf"
> 
> Hello Aleksandr,
> 
> Thank you for your review.
> 
> On 9/3/25 12:07 PM, Loktionov, Aleksandr wrote:
> 
>         -----Original Message-----
> 
>         From:mheib@...hat.com <mailto:mheib@...hat.com> <mheib@...hat.com> <mailto:mheib@...hat.com>
> 
>         Sent: Wednesday, September 3, 2025 9:58 AM
> 
>         To:intel-wired-lan@...ts.osuosl.org <mailto:intel-wired-lan@...ts.osuosl.org>
> 
>         Cc:przemyslawx.patynowski@...el.com <mailto:przemyslawx.patynowski@...el.com>;jiri@...nulli.us <mailto:jiri@...nulli.us>;
> 
>         netdev@...r.kernel.org <mailto:netdev@...r.kernel.org>;horms@...nel.org <mailto:horms@...nel.org>; Keller, Jacob E
> 
>         <jacob.e.keller@...el.com> <mailto:jacob.e.keller@...el.com>; Loktionov, Aleksandr
> 
>         <aleksandr.loktionov@...el.com> <mailto:aleksandr.loktionov@...el.com>; Nguyen, Anthony L
> 
>         <anthony.l.nguyen@...el.com> <mailto:anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> 
>         <przemyslaw.kitszel@...el.com> <mailto:przemyslaw.kitszel@...el.com>; Mohammad Heib<mheib@...hat.com> <mailto:mheib@...hat.com>
> 
>         Subject: [PATCH net-next,2/2] i40e: support generic devlink param
> 
>         "max_mac_per_vf"
> 
>         From: Mohammad Heib<mheib@...hat.com> <mailto:mheib@...hat.com>
> 
>         Add support for the new generic devlink runtime parameter
> 
>         "max_mac_per_vf", which controls the maximum number of MAC addresses a
> 
>         trusted VF can use.
> 
>     Good day Mohammad,
> 
>     Thanks for working on this and for the clear explanation in the commit message.
> 
>     I have a couple of questions and thoughts:
> 
>     1) Scope of the parameter
> 
>          The name max_mac_per_vf is a bit ambiguous. From the description,
> 
>          it seems to apply only to trusted VFs, but the name does not make that obvious.
> 
>          Would it make sense to either:
> 
>       - Make the name reflect that (e.g., max_mac_per_trusted_vf), or
> 
>       - Introduce two separate parameters for trusted and untrusted VFs if both cases need to be handled differently?
> 
> I agree that the name could be a bit confusing. Since this is a generic 
> devlink parameter, different devices may handle trusted and untrusted 
> VFs differently.
> For i40e specifically, the device does treat trusted VFs differently 
> from untrusted ones, and this is documented in devlink/i40e.rst.
> However, I chose a more general name to avoid creating a separate 
> devlink parameter for untrusted VFs, which likely wouldn’t be used.
> On reflection, I should update the patch number 1 to remove the 
> **trusted VF** wording from the description to avoid implying that the 
> parameter only applies to trusted VFs.
> 
>     I believe the community generally aims for solutions that work
>     consistently across different hardware. If this parameter behaves
>     differently on i40e compared to mlx5 (or other drivers), it might be
>     helpful to mention that explicitly in the documentation or commit
>     message.
> 
>     2)Problem statement
> 
>          It would help to better understand the underlying problem this parameter is solving.
> 
>          Is the goal to enforce a global cap for all VFs, or to provide operators with a way
> 
>          to fine-tune per-VF limits? From my perspective, the most important part is
> 
>          clearly stating the problem and the use case.
> 
> My main goal here is to enforce a global cap for all VFs.
> There was a long discussion [1] about this, and one of the ideas raised 
> was to create fine-tuned per-VF limits using devlink resources instead 
> of a parameter
> However, currently in i40e, we only create a devlink port per PF and no 
> devlink ports per VF.
> Implementing the resource-per-VF approach would therefore require some 
> extra work.
> so i decided to go with this global cap for now.
> [1] - https://patchwork.kernel.org/project/netdevbpf/ 
> patch/20250805134042.2604897-2-dhill@...hat.com/ <https:// 
> patchwork.kernel.org/project/netdevbpf/patch/20250805134042.2604897-2- 
> dhill@...hat.com/>
> 
> Thank, you Mohammad
> 
> The https://patchwork.kernel.org/project/netdevbpf/ 
> patch/20250805134042.2604897-2-dhill@...hat.com/ <https:// 
> patchwork.kernel.org/project/netdevbpf/patch/20250805134042.2604897-2- 
> dhill@...hat.com/> explains many things.
> 
> It might be helpful to include a brief description of the problem being 
> solved directly in the commit message. This gives reviewers the 
> necessary context and makes it easier to understand the motivation 
> behind the change.
> 
>     …
> 

Powered by blists - more mailing lists