Message-ID: <IA1PR11MB6169F0DD76B4D34CB1C20190B8489@IA1PR11MB6169.namprd11.prod.outlook.com>
Date: Wed, 31 May 2023 10:54:41 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Vladimir Oltean <vladimir.oltean@....com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, "Hadi Salim, Jamal" <jhs@...atatu.com>, Cong Wang
	<xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, "Gomes, Vinicius"
	<vinicius.gomes@...el.com>, Kurt Kanzenbach <kurt@...utronix.de>, "Gerhard
 Engleder" <gerhard@...leder-embedded.com>, "Nambiar, Amritha"
	<amritha.nambiar@...el.com>, Ferenc Fejes <ferenc.fejes@...csson.com>,
	Xiaoliang Yang <xiaoliang.yang_1@....com>, Roger Quadros <rogerq@...nel.org>,
	Pranavi Somisetty <pranavi.somisetty@....com>, Harini Katakam
	<harini.katakam@....com>, Giuseppe Cavallaro <peppe.cavallaro@...com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>, "Sit, Michael Wei Hong"
	<michael.wei.hong.sit@...el.com>, "Ismail, Mohammad Athari"
	<mohammad.athari.ismail@...el.com>, Oleksij Rempel <linux@...pel-privat.de>,
	"Keller, Jacob E" <jacob.e.keller@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>, "Florian
 Fainelli" <f.fainelli@...il.com>, Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>, "Brandeburg,
 Jesse" <jesse.brandeburg@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Jose Abreu <joabreu@...opsys.com>, Maxime Coquelin
	<mcoquelin.stm32@...il.com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>
Subject: RE: [PATCH net-next 3/5] net/sched: taprio: add netlink reporting for
 offload statistics counters

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@....com>
> Sent: Tuesday, 30 May, 2023 5:20 PM
> To: netdev@...r.kernel.org
> Cc: David S. Miller <davem@...emloft.net>; Eric Dumazet
> <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> <pabeni@...hat.com>; Hadi Salim, Jamal <jhs@...atatu.com>; Cong Wang
> <xiyou.wangcong@...il.com>; Jiri Pirko <jiri@...nulli.us>; Gomes, Vinicius
> <vinicius.gomes@...el.com>; Kurt Kanzenbach <kurt@...utronix.de>;
> Gerhard Engleder <gerhard@...leder-embedded.com>; Nambiar, Amritha
> <amritha.nambiar@...el.com>; Ferenc Fejes <ferenc.fejes@...csson.com>;
> Xiaoliang Yang <xiaoliang.yang_1@....com>; Roger Quadros
> <rogerq@...nel.org>; Pranavi Somisetty <pranavi.somisetty@....com>;
> Harini Katakam <harini.katakam@....com>; Giuseppe Cavallaro
> <peppe.cavallaro@...com>; Alexandre Torgue
> <alexandre.torgue@...s.st.com>; Sit, Michael Wei Hong
> <michael.wei.hong.sit@...el.com>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@...el.com>; Oleksij Rempel <linux@...pel-
> privat.de>; Keller, Jacob E <jacob.e.keller@...el.com>; linux-
> kernel@...r.kernel.org; Andrew Lunn <andrew@...n.ch>; Florian Fainelli
> <f.fainelli@...il.com>; Claudiu Manoil <claudiu.manoil@....com>;
> Alexandre Belloni <alexandre.belloni@...tlin.com>;
> UNGLinuxDriver@...rochip.com; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; Horatiu Vultur
> <horatiu.vultur@...rochip.com>; Jose Abreu <joabreu@...opsys.com>;
> Maxime Coquelin <mcoquelin.stm32@...il.com>; intel-wired-
> lan@...ts.osuosl.org; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@...el.com>
> Subject: [PATCH net-next 3/5] net/sched: taprio: add netlink reporting for
> offload statistics counters
> 
> Offloading drivers may report some additional statistics counters, some of
> them even suggested by 802.1Q, like TransmissionOverrun.
> 
> In my opinion we don't have to limit ourselves to reporting counters only
> globally to the Qdisc/interface, especially if the device has more detailed
> reporting (per traffic class), since the more detailed info is valuable for
> debugging and can help identifying who is exceeding its time slot.
> 
> But on the other hand, some devices may not be able to report both per TC
> and global stats.
> 
> So we end up reporting both ways, and use the good old ethtool_put_stat()
> strategy to determine which statistics are supported by this NIC.
> Statistics which aren't set are simply not reported to netlink. For this reason,
> we need something dynamic (a nlattr nest) to be reported through
> TCA_STATS_APP, and not something daft like the fixed-size and inextensible
> struct tc_codel_xstats. A good model for xstats which are a nlattr nest rather
> than a fixed struct seems to be cake.
> 
>  # Global stats
>  $ tc -s qdisc show dev eth0 root
>  # Per-tc stats
>  $ tc -s class show dev eth0
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Tested-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>

Thanks for the patch :) 
I applied my changes on top of your patch series, and it worked for both 
TAPRIO_CMD_STATS and TAPRIO_CMD_TC_STATS.  Awesome!

> ---
>  include/net/pkt_sched.h        | 47 ++++++++++++++++----
>  include/uapi/linux/pkt_sched.h | 10 +++++
>  net/sched/sch_taprio.c         | 78 +++++++++++++++++++++++++++++++++-
>  3 files changed, 126 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index
> f5fb11da357b..530d33adec88 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -188,6 +188,27 @@ struct tc_taprio_caps {  enum tc_taprio_qopt_cmd {
>  	TAPRIO_CMD_REPLACE,
>  	TAPRIO_CMD_DESTROY,
> +	TAPRIO_CMD_STATS,
> +	TAPRIO_CMD_TC_STATS,
> +};
> +
> +/**
> + * struct tc_taprio_qopt_stats - IEEE 802.1Qbv statistics
> + * @window_drops: Frames that were dropped because they were too large
> to be
> + *	transmitted in any of the allotted time windows (open gates) for their
> + *	traffic class.
> + * @tx_overruns: Frames still being transmitted by the MAC after the
> + *	transmission gate associated with their traffic class has closed.
> + *	Equivalent to `12.29.1.1.2 TransmissionOverrun` from 802.1Q-2018.
> + */
> +struct tc_taprio_qopt_stats {
> +	u64 window_drops;
> +	u64 tx_overruns;
> +};
> +
> +struct tc_taprio_qopt_tc_stats {
> +	int tc;
> +	struct tc_taprio_qopt_stats stats;
>  };
> 
>  struct tc_taprio_sched_entry {
> @@ -199,16 +220,26 @@ struct tc_taprio_sched_entry {  };
> 
>  struct tc_taprio_qopt_offload {
> -	struct tc_mqprio_qopt_offload mqprio;
> -	struct netlink_ext_ack *extack;
>  	enum tc_taprio_qopt_cmd cmd;
> -	ktime_t base_time;
> -	u64 cycle_time;
> -	u64 cycle_time_extension;
> -	u32 max_sdu[TC_MAX_QUEUE];
> 
> -	size_t num_entries;
> -	struct tc_taprio_sched_entry entries[];
> +	union {
> +		/* TAPRIO_CMD_STATS */
> +		struct tc_taprio_qopt_stats stats;
> +		/* TAPRIO_CMD_TC_STATS */
> +		struct tc_taprio_qopt_tc_stats tc_stats;
> +		/* TAPRIO_CMD_REPLACE */
> +		struct {
> +			struct tc_mqprio_qopt_offload mqprio;
> +			struct netlink_ext_ack *extack;
> +			ktime_t base_time;
> +			u64 cycle_time;
> +			u64 cycle_time_extension;
> +			u32 max_sdu[TC_MAX_QUEUE];
> +
> +			size_t num_entries;
> +			struct tc_taprio_sched_entry entries[];
> +		};
> +	};
>  };
> 
>  #if IS_ENABLED(CONFIG_NET_SCH_TAPRIO)
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 51a7addc56c6..00f6ff0aff1f 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -1259,6 +1259,16 @@ enum {
>  	TCA_TAPRIO_TC_ENTRY_MAX = (__TCA_TAPRIO_TC_ENTRY_CNT - 1)
> };
> 
> +enum {
> +	TCA_TAPRIO_OFFLOAD_STATS_PAD = 1,	/* u64 */
> +	TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS,	/* u64 */
> +	TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS,	/* u64 */
> +
> +	/* add new constants above here */
> +	__TCA_TAPRIO_OFFLOAD_STATS_CNT,
> +	TCA_TAPRIO_OFFLOAD_STATS_MAX =
> (__TCA_TAPRIO_OFFLOAD_STATS_CNT - 1) };
> +
>  enum {
>  	TCA_TAPRIO_ATTR_UNSPEC,
>  	TCA_TAPRIO_ATTR_PRIOMAP, /* struct tc_mqprio_qopt */ diff --git
> a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index
> 06bf4c6355a5..3c4c2c334878 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -27,6 +27,8 @@
>  #include <net/sock.h>
>  #include <net/tcp.h>
> 
> +#define TAPRIO_STAT_NOT_SET	(~0ULL)
> +
>  #include "sch_mqprio_lib.h"
> 
>  static LIST_HEAD(taprio_list);
> @@ -2289,6 +2291,72 @@ static int taprio_dump_tc_entries(struct sk_buff
> *skb,
>  	return -EMSGSIZE;
>  }
> 
> +static int taprio_put_stat(struct sk_buff *skb, u64 val, u16 attrtype)
> +{
> +	if (val == TAPRIO_STAT_NOT_SET)
> +		return 0;
> +	if (nla_put_u64_64bit(skb, attrtype, val,
> TCA_TAPRIO_OFFLOAD_STATS_PAD))
> +		return -EMSGSIZE;
> +	return 0;
> +}
> +
> +static int taprio_dump_xstats(struct Qdisc *sch, struct gnet_dump *d,
> +			      struct tc_taprio_qopt_offload *offload,
> +			      struct tc_taprio_qopt_stats *stats) {
> +	struct net_device *dev = qdisc_dev(sch);
> +	const struct net_device_ops *ops;
> +	struct sk_buff *skb = d->skb;
> +	struct nlattr *xstats;
> +	int err;
> +
> +	ops = qdisc_dev(sch)->netdev_ops;
> +
> +	/* FIXME I could use qdisc_offload_dump_helper(), but that messes
> +	 * with sch->flags depending on whether the device reports taprio
> +	 * stats, and I'm not sure whether that's a good idea, considering
> +	 * that stats are optional to the offload itself
> +	 */
> +	if (!ops->ndo_setup_tc)
> +		return 0;
> +
> +	memset(stats, 0xff, sizeof(*stats));
> +
> +	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
> +	if (err == -EOPNOTSUPP)
> +		return 0;
> +	if (err)
> +		return err;
> +
> +	xstats = nla_nest_start(skb, TCA_STATS_APP);
> +	if (!xstats)
> +		goto err;
> +
> +	if (taprio_put_stat(skb, stats->window_drops,
> +			    TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS)
> ||
> +	    taprio_put_stat(skb, stats->tx_overruns,
> +			    TCA_TAPRIO_OFFLOAD_STATS_TX_OVERRUNS))
> +		goto err_cancel;
> +
> +	nla_nest_end(skb, xstats);
> +
> +	return 0;
> +
> +err_cancel:
> +	nla_nest_cancel(skb, xstats);
> +err:
> +	return -EMSGSIZE;
> +}
> +
> +static int taprio_dump_stats(struct Qdisc *sch, struct gnet_dump *d) {
> +	struct tc_taprio_qopt_offload offload = {
> +		.cmd = TAPRIO_CMD_STATS,
> +	};
> +
> +	return taprio_dump_xstats(sch, d, &offload, &offload.stats); }
> +
>  static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)  {
>  	struct taprio_sched *q = qdisc_priv(sch); @@ -2389,11 +2457,18 @@
> static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,  {
>  	struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
>  	struct Qdisc *child = dev_queue->qdisc_sleeping;
> +	struct tc_taprio_qopt_offload offload = {
> +		.cmd = TAPRIO_CMD_TC_STATS,
> +		.tc_stats = {
> +			.tc = cl - 1,
> +		},
> +	};
> 
>  	if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
>  	    qdisc_qstats_copy(d, child) < 0)
>  		return -1;
> -	return 0;
> +
> +	return taprio_dump_xstats(sch, d, &offload, &offload.tc_stats.stats);
>  }
> 
>  static void taprio_walk(struct Qdisc *sch, struct qdisc_walker *arg) @@ -
> 2440,6 +2515,7 @@ static struct Qdisc_ops taprio_qdisc_ops __read_mostly
> = {
>  	.dequeue	= taprio_dequeue,
>  	.enqueue	= taprio_enqueue,
>  	.dump		= taprio_dump,
> +	.dump_stats	= taprio_dump_stats,
>  	.owner		= THIS_MODULE,
>  };
> 
> --
> 2.34.1

Powered by blists - more mailing lists