netdev - Re: [patch net-next 5/5] mlxsw: spectrum: qdiscs: Support stats for PRIO qdisc

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: <20180111160726.57a7ae5b@cakuba.netronome.com>
Date:   Thu, 11 Jan 2018 16:07:50 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, nogahf@...lanox.com, yuvalm@...lanox.com,
        davem@...emloft.net, idosch@...lanox.com, mlxsw@...lanox.com,
        jhs@...atatu.com, xiyou.wangcong@...il.com
Subject: Re: [patch net-next 5/5] mlxsw: spectrum: qdiscs: Support stats for
 PRIO qdisc

On Thu, 11 Jan 2018 11:21:02 +0100, Jiri Pirko wrote:
> From: Nogah Frankel <nogahf@...lanox.com>
> 
> Support basic stats for PRIO qdisc, which includes tx packets and bytes
> count, drops count and backlog size. The rest of the stats are irrelevant
> for this qdisc offload.
> Since backlog is not only incremental but reflecting momentary value, in
> case of a qdisc that stops being offloaded but is not destroyed, backlog
> value needs to be updated about the un-offloading.
> For that reason an unoffload function is being added to the ops struct.
> 
> Signed-off-by: Nogah Frankel <nogahf@...lanox.com>
> Reviewed-by: Yuval Mintz <yuvalm@...lanox.com>
> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> ---
>  .../net/ethernet/mellanox/mlxsw/spectrum_qdisc.c   | 92 ++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
> index 9e83edde7b35..272c04951e5d 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
> @@ -66,6 +66,11 @@ struct mlxsw_sp_qdisc_ops {
>  			  void *xstats_ptr);
>  	void (*clean_stats)(struct mlxsw_sp_port *mlxsw_sp_port,
>  			    struct mlxsw_sp_qdisc *mlxsw_sp_qdisc);
> +	/* unoffload - to be used for a qdisc that stops being offloaded without
> +	 * being destroyed.
> +	 */
> +	void (*unoffload)(struct mlxsw_sp_port *mlxsw_sp_port,
> +			  struct mlxsw_sp_qdisc *mlxsw_sp_qdisc, void *params);

Hm.  You you need this just because you didn't add the backlog pointer
to destroy?  AFAIK on destroy we are free to reset stats as well, thus
simplifying your driver...  Let me know if I misunderstand.

>  };
>  
>  struct mlxsw_sp_qdisc {
> @@ -73,6 +78,9 @@ struct mlxsw_sp_qdisc {
>  	u8 tclass_num;
>  	union {
>  		struct red_stats red;
> +		struct mlxsw_sp_qdisc_prio_stats {
> +			u64 backlog;

This is not a prio stat, it's a standard qstat.  I've added it to
struct mlxsw_sp_qdisc_stats.  The reason you need to treat it
separately is that RED has non-standard backlog handling which I'm
trying to fix...

> +		} prio;
>  	} xstats_base;
>  	struct mlxsw_sp_qdisc_stats {
>  		u64 tx_bytes;
> @@ -144,6 +152,9 @@ mlxsw_sp_qdisc_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
>  
>  err_bad_param:
>  err_config:
> +	if (mlxsw_sp_qdisc->handle == handle && ops->unoffload)
> +		ops->unoffload(mlxsw_sp_port, mlxsw_sp_qdisc, params);
> +
>  	mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc);
>  	return err;
>  }

> @@ -479,6 +567,10 @@ int mlxsw_sp_setup_tc_prio(struct mlxsw_sp_port *mlxsw_sp_port,
>  	switch (p->command) {
>  	case TC_PRIO_DESTROY:
>  		return mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc);
> +	case TC_PRIO_STATS:
> +		return mlxsw_sp_qdisc_get_stats(mlxsw_sp_port, mlxsw_sp_qdisc,
> +						&p->stats);
> +

nit: extra new line intentional? :)

>  	default:
>  		return -EOPNOTSUPP;
>  	}