On 9/10/24 10:29, Loktionov, Aleksandr wrote:
From: Fijalkowski, Maciej <maciej.fijalkow...@intel.com>
Sent: Tuesday, September 3, 2024 9:59 PM
To: Loktionov, Aleksandr <aleksandr.loktio...@intel.com>
Cc: intel-wired-...@lists.osuosl.org; Nguyen, Anthony L
<anthony.l.ngu...@intel.com>; net...@vger.kernel.org; Sokolowski, Jan
<jan.sokolow...@intel.com>; Connolly, Padraig J
<padraig.j.conno...@intel.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.sokolow...@intel.com>
Signed-off-by: Jan Sokolowski <jan.sokolow...@intel.com>
Co-developed-by: Padraig J Connolly <padraig.j.conno...@intel.com>
Signed-off-by: Padraig J Connolly <padraig.j.conno...@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktio...@intel.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()
