Re: [PATCH net] net/mlx5e: limit log messages due to (ovs) probing to _once

[Saeed Mahameed <saeedm@mellanox.com>]

On Thu, 2020-04-02 at 21:11 -0300, Marcelo Ricardo Leitner wrote:
> OVS will keep adding such flows, no matter what. They will usually be
> handled by tc software (or ovs datapath, if skip_sw is used). But the
> driver is logging these messages for each and every attempt, despite
> the
> extack. Note that they weren't rate limited, and a broadcast storm
> could
> trigger system console flooding.
> 
> Switch these to be _once. It's enough to tell the sysadmin what is
> happenning, and if anything, the OVS log will have all the errors.
> 

++ mlnx TC stake holders 

The fact that for all of the suppressed messages we will still have NL
extack reporting, makes it easier for me to agree with this patch. but
there is a loss of information since now we will stop printing the
attribute/params which caused the failure in most of the cases, and it
will be harder for the user and the developer to understand why these
attributes are not working .. 

I understand it is for debug only but i strongly suggest to not totally
suppress these messages and maybe just move them to tracepoints buffer
? for those who would want to really debug .. 

we already have some tracepoints implemented for en_tc.c 
mlx5/core/diag/en_tc_tracepoints.c, maybe we should define a tracepoint
for error reporting .. 

> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

net patches must have a "Fixes:" tag

> ---
>  .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 61 ++++++++++-------
> --
>  1 file changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index
> 438128dde187d7ec58892c2879c6037f807f576f..1182fba3edbb8cf7bd59557b7ec
> e18765c704186 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -1828,8 +1828,8 @@ enc_opts_is_dont_care_or_full_match(struct
> mlx5e_priv *priv,
>  				       opt->length * 4)) {
>  				NL_SET_ERR_MSG(extack,
>  					       "Partial match of tunnel
> options in chain > 0 isn't supported");
> -				netdev_warn(priv->netdev,
> -					    "Partial match of tunnel
> options in chain > 0 isn't supported");
> +				netdev_warn_once(priv->netdev,
> +						 "Partial match of
> tunnel options in chain > 0 isn't supported");
>  				return -EOPNOTSUPP;
>  			}
>  		}
> @@ -1988,8 +1988,8 @@ static int parse_tunnel_attr(struct mlx5e_priv
> *priv,
>  	    !mlx5_eswitch_reg_c1_loopback_enabled(esw)) {
>  		NL_SET_ERR_MSG(extack,
>  			       "Chains on tunnel devices isn't
> supported without register loopback support");
> -		netdev_warn(priv->netdev,
> -			    "Chains on tunnel devices isn't supported
> without register loopback support");
> +		netdev_warn_once(priv->netdev,
> +				 "Chains on tunnel devices isn't
> supported without register loopback support");
>  		return -EOPNOTSUPP;
>  	}
>  
> @@ -2133,8 +2133,8 @@ static int __parse_cls_flower(struct mlx5e_priv
> *priv,
>  	      BIT(FLOW_DISSECTOR_KEY_ENC_IP) |
>  	      BIT(FLOW_DISSECTOR_KEY_ENC_OPTS))) {
>  		NL_SET_ERR_MSG_MOD(extack, "Unsupported key");
> -		netdev_warn(priv->netdev, "Unsupported key used:
> 0x%x\n",
> -			    dissector->used_keys);
> +		netdev_warn_once(priv->netdev, "Unsupported key used:
> 0x%x\n",
> +				 dissector->used_keys);
>  		return -EOPNOTSUPP;
>  	}
>  
> @@ -2484,8 +2484,8 @@ static int parse_cls_flower(struct mlx5e_priv
> *priv,
>  		    esw->offloads.inline_mode <
> non_tunnel_match_level)) {
>  			NL_SET_ERR_MSG_MOD(extack,
>  					   "Flow is not offloaded due
> to min inline setting");
> -			netdev_warn(priv->netdev,
> -				    "Flow is not offloaded due to min
> inline setting, required %d actual %d\n",
> +			netdev_warn_once(priv->netdev,
> +					 "Flow is not offloaded due to
> min inline setting, required %d actual %d\n",
>  				    non_tunnel_match_level, esw-
> >offloads.inline_mode);
>  			return -EOPNOTSUPP;
>  		}
> @@ -2885,7 +2885,9 @@ static int alloc_tc_pedit_action(struct
> mlx5e_priv *priv, int namespace,
>  		if (memcmp(cmd_masks, &zero_masks, sizeof(zero_masks)))
> {
>  			NL_SET_ERR_MSG_MOD(extack,
>  					   "attempt to offload an
> unsupported field");
> -			netdev_warn(priv->netdev, "attempt to offload
> an unsupported field (cmd %d)\n", cmd);
> +			netdev_warn_once(priv->netdev,
> +					 "attempt to offload an
> unsupported field (cmd %d)\n",
> +					 cmd);
>  			print_hex_dump(KERN_WARNING, "mask: ",
> DUMP_PREFIX_ADDRESS,
>  				       16, 1, cmd_masks,
> sizeof(zero_masks), true);
>  			err = -EOPNOTSUPP;
> @@ -2912,17 +2914,17 @@ static bool csum_offload_supported(struct
> mlx5e_priv *priv,
>  	if (!(action & MLX5_FLOW_CONTEXT_ACTION_MOD_HDR)) {
>  		NL_SET_ERR_MSG_MOD(extack,
>  				   "TC csum action is only offloaded
> with pedit");
> -		netdev_warn(priv->netdev,
> -			    "TC csum action is only offloaded with
> pedit\n");
> +		netdev_warn_once(priv->netdev,
> +				 "TC csum action is only offloaded with
> pedit\n");
>  		return false;
>  	}
>  
>  	if (update_flags & ~prot_flags) {
>  		NL_SET_ERR_MSG_MOD(extack,
>  				   "can't offload TC csum action for
> some header/s");
> -		netdev_warn(priv->netdev,
> -			    "can't offload TC csum action for some
> header/s - flags %#x\n",
> -			    update_flags);
> +		netdev_warn_once(priv->netdev,
> +				 "can't offload TC csum action for some
> header/s - flags %#x\n",
> +				 update_flags);
>  		return false;
>  	}
>  
> @@ -3224,8 +3226,9 @@ static int parse_tc_nic_actions(struct
> mlx5e_priv *priv,
>  			} else {
>  				NL_SET_ERR_MSG_MOD(extack,
>  						   "device is not on
> same HW, can't offload");
> -				netdev_warn(priv->netdev, "device %s
> not on same HW, can't offload\n",
> -					    peer_dev->name);
> +				netdev_warn_once(priv->netdev,
> +						 "device %s not on same
> HW, can't offload\n",
> +						 peer_dev->name);
>  				return -EINVAL;
>  			}
>  			}
> @@ -3754,9 +3757,9 @@ static int parse_tc_fdb_actions(struct
> mlx5e_priv *priv,
>  			if (attr->out_count >=
> MLX5_MAX_FLOW_FWD_VPORTS) {
>  				NL_SET_ERR_MSG_MOD(extack,
>  						   "can't support more
> output ports, can't offload forwarding");
> -				netdev_warn(priv->netdev,
> -					    "can't support more than %d
> output ports, can't offload forwarding\n",
> -					    attr->out_count);
> +				netdev_warn_once(priv->netdev,
> +						 "can't support more
> than %d output ports, can't offload forwarding\n",
> +						 attr->out_count);
>  				return -EOPNOTSUPP;
>  			}
>  
> @@ -3821,10 +3824,10 @@ static int parse_tc_fdb_actions(struct
> mlx5e_priv *priv,
>  				if
> (!mlx5e_is_valid_eswitch_fwd_dev(priv, out_dev)) {
>  					NL_SET_ERR_MSG_MOD(extack,
>  							   "devices are
> not on same switch HW, can't offload forwarding");
> -					netdev_warn(priv->netdev,
> -						    "devices %s %s not
> on same switch HW, can't offload forwarding\n",
> -						    priv->netdev->name,
> -						    out_dev->name);
> +					netdev_warn_once(priv->netdev,
> +							 "devices %s %s
> not on same switch HW, can't offload forwarding\n",
> +							 priv->netdev-
> >name,
> +							 out_dev-
> >name);
>  					return -EOPNOTSUPP;
>  				}
>  
> @@ -3843,10 +3846,10 @@ static int parse_tc_fdb_actions(struct
> mlx5e_priv *priv,
>  			} else {
>  				NL_SET_ERR_MSG_MOD(extack,
>  						   "devices are not on
> same switch HW, can't offload forwarding");
> -				netdev_warn(priv->netdev,
> -					    "devices %s %s not on same
> switch HW, can't offload forwarding\n",
> -					    priv->netdev->name,
> -					    out_dev->name);
> +				netdev_warn_once(priv->netdev,
> +						 "devices %s %s not on
> same switch HW, can't offload forwarding\n",
> +						 priv->netdev->name,
> +						 out_dev->name);
>  				return -EINVAL;
>  			}
>  			}
> @@ -3959,8 +3962,8 @@ static int parse_tc_fdb_actions(struct
> mlx5e_priv *priv,
>  
>  			NL_SET_ERR_MSG(extack,
>  				       "Decap with goto isn't
> supported");
> -			netdev_warn(priv->netdev,
> -				    "Decap with goto isn't supported");
> +			netdev_warn_once(priv->netdev,
> +					 "Decap with goto isn't
> supported");
>  			return -EOPNOTSUPP;
>  		}
>