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: <VI1PR05MB35201776F2FCD9F7821B1962BF170@VI1PR05MB3520.eurprd05.prod.outlook.com>
Date:   Fri, 12 Jan 2018 00:39:26 +0000
From:   Yuval Mintz <yuvalm@...lanox.com>
To:     Jakub Kicinski <kubakici@...pl>, Jiri Pirko <jiri@...nulli.us>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Nogah Frankel <nogahf@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Ido Schimmel" <idosch@...lanox.com>, mlxsw <mlxsw@...lanox.com>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: RE: [patch net-next 5/5] mlxsw: spectrum: qdiscs: Support stats for
 PRIO qdisc

> > 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.

This is meant exactly for the scenario where qdisc didn't get destroyed
yet is no longer offloaded; E.g., if number of bands increased beyond
What we can offload. So we can't reset the statistics in this case.
[Although I might be the one to misunderstand you,
as the 'not destroyed' was explicitly mentioned twice above]

> 
> >  };
> >
> >  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;
> >  	}