netdev - Re: [Intel-wired-lan] [PATCH] i40e: avoid redundant VF link state updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CADcc-bxT13tqWKQFfXX6a5R125dRT21VT+5_ozzV-pmpX708gA@mail.gmail.com>
Date: Wed, 22 Oct 2025 18:11:47 +0200
From: Robert Malz <robert.malz@...onical.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
Cc: Aleksandr Loktionov <aleksandr.loktionov@...el.com>, intel-wired-lan@...ts.osuosl.org, 
	netdev@...r.kernel.org, Jamie Bainbridge <jamie.bainbridge@...il.com>, 
	Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>, Dennis Chen <dechen@...hat.com>, 
	Przemyslaw Kitszel <przemyslaw.kitszel@...el.com>, Lukasz Czapnik <lukasz.czapnik@...el.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, Eric Dumazet <edumazet@...gle.com>, 
	Anthony L Nguyen <anthony.l.nguyen@...el.com>, Simon Horman <horms@...nel.org>, 
	"Keller, Jacob E" <jacob.e.keller@...el.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, "David S . Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH] i40e: avoid redundant VF link state updates

Hey Paul, Aleksandr
Thanks for the feedback.
I have updated the commit message in [PATCH v2], some notes inline.

On Wed, Oct 22, 2025 at 2:25 PM Paul Menzel <pmenzel@...gen.mpg.de> wrote:
>
> Dear Alex,
>
>
> Thank you for your input.
>
> Am 22.10.25 um 14:06 schrieb Loktionov, Aleksandr:
>
> >> -----Original Message-----
> >> From: Paul Menzel <pmenzel@...gen.mpg.de>
> >> Sent: Wednesday, October 22, 2025 1:49 PM
>
> >> Am 21.10.25 um 17:44 schrieb Robert Malz:
> >>> From: Jay Vosburgh <jay.vosburgh@...onical.com>
> >>>
> >>> Multiple sources can request VF link state changes with identical
> >>> parameters. For example, Neutron may request to set the VF link state
> >>> to
> >>
> >> What is Neutron?
> >>
> >>> IFLA_VF_LINK_STATE_AUTO during every initialization or user can issue:
> >>> `ip link set <ifname> vf 0 state auto` multiple times. Currently, the
> >>> i40e driver processes each of these requests, even if the requested
> >>> state is the same as the current one. This leads to unnecessary VF
> >>> resets and can cause performance degradation or instability in the VF
> >>> driver - particularly in DPDK environment.
> >>
> >> What is DPDK?
> >>
> > I think Robert needs:
> > - to expand acronyms in the commit message (Neutron → OpenStack Neutron, DPDK → Data Plane Development Kit).
> > - to fix the comment style as per coding guidelines.
> > - add a short note in the commit message about how to reproduce the issue.
> > @Paul Menzel right?
>

@Aleksandr Loktionov you mentioned that comment style does not follow
coding guidelines, from my perspective it looks good.
Could you elaborate on that point?

> Correct.
>
> Maybe also mention how to force it, as there seems to be such an option
> judging from the diff.
>
> >>> With this patch i40e will skip VF link state change requests when the
> >>> desired link state matches the current configuration. This prevents
> >>> unnecessary VF resets and reduces PF-VF communication overhead.
> >>
> >> Add a test (with `ip link …`) case to show, that it works now.
> >>
> >>> Co-developed-by: Robert Malz <robert.malz@...onical.com>
> >>> Signed-off-by: Robert Malz <robert.malz@...onical.com>
> >>> Signed-off-by: Jay Vosburgh <jay.vosburgh@...onical.com>
> >>> ---
> >>>    drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 12 ++++++++++++
> >>>    1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>> index 081a4526a2f0..0fe0d52c796b 100644
> >>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>> @@ -4788,6 +4788,7 @@ int i40e_ndo_set_vf_link_state(struct net_device *netdev, int vf_id, int link)
> >>>     unsigned long q_map;
> >>>     struct i40e_vf *vf;
> >>>     int abs_vf_id;
> >>> +   int old_link;
> >>>     int ret = 0;
> >>>     int tmp;
> >>>
> >>> @@ -4806,6 +4807,17 @@ int i40e_ndo_set_vf_link_state(struct net_device *netdev, int vf_id, int link)
> >>>     vf = &pf->vf[vf_id];
> >>>     abs_vf_id = vf->vf_id + hw->func_caps.vf_base_id;
> >>>
> >>> +   /* skip VF link state change if requested state is already set */
> >>> +   if (!vf->link_forced)
> >>> +           old_link = IFLA_VF_LINK_STATE_AUTO;
> >>> +   else if (vf->link_up)
> >>> +           old_link = IFLA_VF_LINK_STATE_ENABLE;
> >>> +   else
> >>> +           old_link = IFLA_VF_LINK_STATE_DISABLE;
> >>> +
> >>> +   if (link == old_link)
> >>> +           goto error_out;
> >>
> >> Should a debug message be added?
> >
> > I think adding one would be redundant since skipping identical state
> > changes is expected behavior.
>
> My thinking was, if something does not work as expected for a user, like
> issuing the command to force a reset, that it might be useful to see
> something in the logs.

I treat VF reset in this scenario as a side effect which helps VF
bring up the queues.
User should not expect to see VF reset each time these commands are
run and there are
specific commands, like triggering VFLR through pci reset, which are
available for that
purpose.
Like Aleksandr, I found the logs in this area to be redundant.
>
> >>> +
> >>>     pfe.event = VIRTCHNL_EVENT_LINK_CHANGE;
> >>>     pfe.severity = PF_EVENT_SEVERITY_INFO;
>
> Kind regards,
>
> Paul

Thanks,
Robert