Message-ID: <SJ0PR11MB58669687E3249583A8209215E5472@SJ0PR11MB5866.namprd11.prod.outlook.com>
Date: Thu, 17 Oct 2024 19:29:25 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>, "Fijalkowski,
 Maciej" <maciej.fijalkowski@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Sokolowski, Jan" <jan.sokolowski@...el.com>,
	"Connolly, Padraig J" <padraig.j.connolly@...el.com>
Subject: RE: [PATCH iwl-next v3] i40e: add ability to reset vf for tx and rx
 mdd events



> -----Original Message-----
> From: Loktionov, Aleksandr
> Sent: Thursday, October 17, 2024 12:02 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>;
> Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org; Sokolowski,
> Jan <jan.sokolowski@...el.com>; Connolly, Padraig J
> <Padraig.J.Connolly@...el.com>
> Subject: RE: [PATCH iwl-next v3] i40e: add ability to reset vf for
> tx and rx mdd events
> 
> 
> 
> > -----Original Message-----
> > From: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>
> > Sent: Tuesday, September 10, 2024 1:11 PM
> > To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>;
> > Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> > Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> > <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org; Sokolowski,
> > Jan <jan.sokolowski@...el.com>; Connolly, Padraig J
> > <padraig.j.connolly@...el.com>
> > Subject: Re: [PATCH iwl-next v3] i40e: add ability to reset vf
> for
> > tx and rx mdd events
> >
> > On 9/10/24 10:29, Loktionov, Aleksandr wrote:
> > >> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> > >> Sent: Tuesday, September 3, 2024 9:59 PM
> > >> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
> > >> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> > >> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org;
> > Sokolowski, Jan
> > >> <jan.sokolowski@...el.com>; Connolly, Padraig J
> > >> <padraig.j.connolly@...el.com>
> > >> Subject: Re: [PATCH iwl-next v3] i40e: add ability to reset vf
> > for tx
> > >> and rx mdd events
> >
> > please capitalize acronyms (Tx, Rx, VF, MDD, PF) (also in the
> > subject line, but sent next version as v4).
> >
> > >>
> > >> On Fri, Aug 30, 2024 at 09:28:07PM +0200, Aleksandr Loktionov
> > wrote:
> > >>> In cases when vf sends malformed packets that are classified
> as
> > >>> malicious, sometimes it causes tx queue to freeze. This
> frozen
> > queue
> > >>> can be stuck for several minutes being unusable. When mdd
> event
> > >>> occurs, there is a posibility to perform a graceful vf reset
> to
> > >>> quickly bring vf back to operational state.
> > >>>
> > >>> Currently vf iqueues are being disabled if mdd event occurs.
> > >>> Add the ability to reset vf if tx or rx mdd occurs.
> > >>> Add mdd events logging throttling /* avoid dmesg polution */.
> > >>> Unify tx rx mdd messages formats.
> > >>>
> > >>> Co-developed-by: Jan Sokolowski <jan.sokolowski@...el.com>
> > >>> Signed-off-by: Jan Sokolowski <jan.sokolowski@...el.com>
> > >>> Co-developed-by: Padraig J Connolly
> > <padraig.j.connolly@...el.com>
> > >>> Signed-off-by:  Padraig J Connolly
> > <padraig.j.connolly@...el.com>
> > >>> Signed-off-by: Aleksandr Loktionov
> > <aleksandr.loktionov@...el.com>
> >
> > Next time please wait for review on our internal e1000-mailing-
> > list.
> > Feel free to ping me directly if there will be no one engaged in
> > any future series of yours.
> >
> > >>> ---
> > >>> v2->v3 fix compilation issue
> > >>> v1->v2 fix compilation issue
> > >>> ---
> > >>>   drivers/net/ethernet/intel/i40e/i40e.h        |   4 +-
> > >>>   .../net/ethernet/intel/i40e/i40e_debugfs.c    |   2 +-
> > >>>   .../net/ethernet/intel/i40e/i40e_ethtool.c    |   2 +
> > >>>   drivers/net/ethernet/intel/i40e/i40e_main.c   | 116
> > >> ++++++++++++++++--
> > >>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    |   2 +-
> > >>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  11 +-
> > >>>   6 files changed, 122 insertions(+), 15 deletions(-)
> > >>>
> >
> >
> > >>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > >>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > >>> @@ -459,6 +459,8 @@ static const struct i40e_priv_flags
> > >> i40e_gstrings_priv_flags[] = {
> > >>>   	I40E_PRIV_FLAG("base-r-fec", I40E_FLAG_BASE_R_FEC, 0),
> > >>>   	I40E_PRIV_FLAG("vf-vlan-pruning",
> > >>>   		       I40E_FLAG_VF_VLAN_PRUNING_ENA, 0),
> > >>> +	I40E_PRIV_FLAG("mdd-auto-reset-vf",
> > >>> +		       I40E_FLAG_MDD_AUTO_RESET_VF, 0),
> > >>
> > >> you don't tell us that this is implemented via priv-flag in
> the
> > >> commit message, would be good to include info about it.
> > > This flag is implemented for other network adapters like ice,
> we
> > thought it's kind of standard.
> > > Can you suggest what exact part to change? Please be concrete.
> > > Thank you
> >
> > priv-flag is not a standard, by definition what we do in intel
> > drivers is also not necessarily a standard
> >
> > keeping the code quality as-is should be rather seen as an
> > allowance for legacy drivers, instead of something that should be
> > copy-pasted yet again. But commit messages are different, you
> need
> > to obey the current standard, which is simply: describe non-
> obvious
> > things, describe more if asked during review. Please do so :)
> >
> > >>> +static void i40e_print_vf_rx_mdd_event(struct i40e_pf *pf,
> > struct
> > >>> +i40e_vf *vf) {
> > >>> +	dev_err(&pf->pdev->dev, "%lld Rx Malicious Driver
> Detection
> > >> events detected on PF %d VF %d MAC %pm. mdd-auto-reset-
> > vfs=%s\n",
> > >>> +		vf->mdd_rx_events.count,
> > >>> +		pf->hw.pf_id,
> > >>> +		vf->vf_id,
> > >>> +		vf->default_lan_addr.addr,
> > >>> +		test_bit(I40E_FLAG_MDD_AUTO_RESET_VF, pf->flags)
> ? "on"
> > :
> > >> "off"); }
> > >>> +
> > >>> +/**
> > >>> + * i40e_print_vf_tx_mdd_event - print VF Tx malicious driver
> > detect
> > >>> +event
> > >>> + * @pf: board private structure
> > >>> + * @vf: pointer to the VF structure  */ static void
> > >>> +i40e_print_vf_tx_mdd_event(struct i40e_pf *pf, struct
> i40e_vf
> > *vf)
> > >>> +{
> > >>> +	dev_err(&pf->pdev->dev, "%lld Tx Malicious Driver
> Detection
> > >> events detected on PF %d VF %d MAC %pm. mdd-auto-reset-
> > vfs=%s\n",
> > >>> +		vf->mdd_tx_events.count,
> > >>> +		pf->hw.pf_id,
> > >>> +		vf->vf_id,
> > >>> +		vf->default_lan_addr.addr,
> > >>> +		test_bit(I40E_FLAG_MDD_AUTO_RESET_VF, pf->flags)
> ? "on"
> > :
> > >> "off"); }
> > >>
> > >> Unnecesary code duplication, two functions with printing the
> > very
> > >> same statement with a single different letter.
> > > But it's easy to grep and find as required by linux coding
> > standards.
> >
> > You could reword to have it still obvious what to grep for, like:
> >
> > Malicious Driver Detected an Event, PF: %d, VF: %d, MAC: %pm,
> dir:
> > %s...
> >
> > with the last %s being "Tx" or "Rx"
> > (note: I didn't copied all your stuff as this is just an example)
> >
> > >>> +
> > >>> +	/* VF MDD event logs are rate limited to one second
> intervals
> > */
> > >>> +	if (time_is_after_jiffies(pf->last_printed_mdd_jiffies
> + HZ *
> > >> 1))
> > >>> +		return;
> > >>> +
> > >>> +	pf->last_printed_mdd_jiffies = jiffies;
> > >>
> > >> why homegrown rate limiting?
> > > Because it works! And other ideas probably didn't.
> > > What is your suggestion exactly? Please be concrete.
> >
> > dev_info_ratelimited()
It doesn't fit requirements. We need custom rate limiting, please see the code!

Powered by blists - more mailing lists