netdev - Re: [PATCH net-next] mlx5: count all link events

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <155D8D8E-C0FE-4EF9-AD7F-B496A8279F92@linux.vnet.ibm.com>
Date:   Wed, 19 May 2021 14:34:34 -0500
From:   Lijun Pan <ljp@...ux.vnet.ibm.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     saeedm@...dia.com, David Miller <davem@...emloft.net>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] mlx5: count all link events



> On May 19, 2021, at 12:18 PM, Jakub Kicinski <kuba@...nel.org> wrote:
> 
> mlx5 devices were observed generating MLX5_PORT_CHANGE_SUBTYPE_ACTIVE
> events without an intervening MLX5_PORT_CHANGE_SUBTYPE_DOWN. This
> breaks link flap detection based on Linux carrier state transition
> count as netif_carrier_on() does nothing if carrier is already on.
> Make sure we count such events.
> 
> netif_carrier_event() increments the counters and fires the linkwatch
> events. The latter is not necessary for the use case but seems like
> the right thing to do.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_main.c  |  6 +++++-
> include/linux/netdevice.h                      |  2 +-
> net/sched/sch_generic.c                        | 18 ++++++++++++++++++
> 3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index bca832cdc4cb..5a67ebc0c96c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -91,12 +91,16 @@ void mlx5e_update_carrier(struct mlx5e_priv *priv)
> {
> 	struct mlx5_core_dev *mdev = priv->mdev;
> 	u8 port_state;
> +	bool up;
> 
> 	port_state = mlx5_query_vport_state(mdev,
> 					    MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT,
> 					    0);
> 
> -	if (port_state == VPORT_STATE_UP) {
> +	up = port_state == VPORT_STATE_UP;
> +	if (up == netif_carrier_ok(priv->netdev))
> +		netif_carrier_event(priv->netdev);
> +	if (up) {
> 		netdev_info(priv->netdev, "Link up\n");
> 		netif_carrier_on(priv->netdev);
> 	} else {
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5cbc950b34df..be1dcceda5e4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4187,8 +4187,8 @@ unsigned long dev_trans_start(struct net_device *dev);
> void __netdev_watchdog_up(struct net_device *dev);
> 
> void netif_carrier_on(struct net_device *dev);
> -
> void netif_carrier_off(struct net_device *dev);
> +void netif_carrier_event(struct net_device *dev);
> 
> /**
>  *	netif_dormant_on - mark device as dormant.
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 44991ea726fc..3090ae32307b 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -515,6 +515,24 @@ void netif_carrier_off(struct net_device *dev)
> }
> EXPORT_SYMBOL(netif_carrier_off);
> 
> +/**
> + *	netif_carrier_event - report carrier state event
> + *	@dev: network device
> + *
> + * Device has detected a carrier event but the carrier state wasn't changed.
> + * Use in drivers when querying carrier state asynchronously, to avoid missing
> + * events (link flaps) if link recovers before it's queried.
> + */
> +void netif_carrier_event(struct net_device *dev)
> +{
> +	if (dev->reg_state == NETREG_UNINITIALIZED)
> +		return;
> +	atomic_inc(&dev->carrier_up_count);
> +	atomic_inc(&dev->carrier_down_count);
> +	linkwatch_fire_event(dev);
> +}
> +EXPORT_SYMBOL_GPL(netif_carrier_event);

Is it possible to integrate netif_carrier_event into netif_carrier_on? like,


void netif_carrier_on(struct net_device *dev)
{
	if (test_and_clear_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
		if (dev->reg_state == NETREG_UNINITIALIZED)
			return;
		atomic_inc(&dev->carrier_up_count);
		linkwatch_fire_event(dev);
		if (netif_running(dev))
			__netdev_watchdog_up(dev);
	} else {
		if (dev->reg_state == NETREG_UNINITIALIZED)
			return;
		atomic_inc(&dev->carrier_down_count);
		atomic_inc(&dev->carrier_up_count);
	}
}
EXPORT_SYMBOL(netif_carrier_on);