As a result of this discussion:
https://lore.kernel.org/intel-wired-lan/[email protected]/
it became apparent that tc-taprio should make an effort to standardize
statistics counters related to the 802.1Qbv scheduling as implemented
by the NIC. I'm presenting here one counter suggested by the standard,
and one counter defined by the NXP ENETC controller from LS1028A. Both
counters are reported globally and per traffic class - drivers get
different callbacks for reporting both of these, and get to choose what
to report in both cases.
The iproute2 counterpart is available here for testing:
https://github.com/vladimiroltean/iproute2/commits/taprio-xstats
Vladimir Oltean (5):
net/sched: taprio: don't overwrite "sch" variable in
taprio_dump_class_stats()
net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a
"cmd" enum
net/sched: taprio: add netlink reporting for offload statistics
counters
net: enetc: refactor enetc_setup_tc_taprio() to have a switch/case for
cmd
net: enetc: report statistics counters for taprio
drivers/net/dsa/hirschmann/hellcreek.c | 14 ++-
drivers/net/dsa/ocelot/felix_vsc9959.c | 4 +-
drivers/net/dsa/sja1105/sja1105_tas.c | 7 +-
.../net/ethernet/engleder/tsnep_selftests.c | 12 +-
drivers/net/ethernet/engleder/tsnep_tc.c | 4 +-
drivers/net/ethernet/freescale/enetc/enetc.c | 3 +-
drivers/net/ethernet/freescale/enetc/enetc.h | 1 +
.../net/ethernet/freescale/enetc/enetc_qos.c | 110 ++++++++++++++----
drivers/net/ethernet/intel/igc/igc_main.c | 13 ++-
.../ethernet/microchip/lan966x/lan966x_tc.c | 10 +-
.../net/ethernet/stmicro/stmmac/stmmac_tc.c | 7 +-
drivers/net/ethernet/ti/am65-cpsw-qos.c | 11 +-
include/net/pkt_sched.h | 56 +++++++--
include/uapi/linux/pkt_sched.h | 10 ++
net/sched/sch_taprio.c | 90 ++++++++++++--
15 files changed, 286 insertions(+), 66 deletions(-)
--
2.34.1
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 <[email protected]>
---
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
Report the "win_drop" counter from the unstructured ethtool -S as
TCA_TAPRIO_OFFLOAD_STATS_WINDOW_DROPS to the Qdisc layer. It is
available both as a global counter as well as a per-TC one.
Signed-off-by: Vladimir Oltean <[email protected]>
---
.../net/ethernet/freescale/enetc/enetc_qos.c | 35 +++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 2b8fdfffd02d..71157eba1fbe 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -147,6 +147,35 @@ static void enetc_taprio_destroy(struct net_device *ndev)
enetc_reset_tc_mqprio(ndev);
}
+static void enetc_taprio_stats(struct net_device *ndev,
+ struct tc_taprio_qopt_stats *stats)
+{
+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
+ u64 window_drops = 0;
+ int i;
+
+ for (i = 0; i < priv->num_tx_rings; i++)
+ window_drops += priv->tx_ring[i]->stats.win_drop;
+
+ stats->window_drops = window_drops;
+}
+
+static void enetc_taprio_tc_stats(struct net_device *ndev,
+ struct tc_taprio_qopt_tc_stats *tc_stats)
+{
+ struct tc_taprio_qopt_stats *stats = &tc_stats->stats;
+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
+ int tc = tc_stats->tc;
+ u64 window_drops = 0;
+ int i;
+
+ for (i = 0; i < priv->num_tx_rings; i++)
+ if (priv->tx_ring[i]->prio == tc)
+ window_drops += priv->tx_ring[i]->stats.win_drop;
+
+ stats->window_drops = window_drops;
+}
+
static int enetc_taprio_replace(struct net_device *ndev,
struct tc_taprio_qopt_offload *offload)
{
@@ -176,6 +205,12 @@ int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
case TAPRIO_CMD_DESTROY:
enetc_taprio_destroy(ndev);
break;
+ case TAPRIO_CMD_STATS:
+ enetc_taprio_stats(ndev, &offload->stats);
+ break;
+ case TAPRIO_CMD_TC_STATS:
+ enetc_taprio_tc_stats(ndev, &offload->tc_stats);
+ break;
default:
err = -EOPNOTSUPP;
}
--
2.34.1
Vladimir Oltean <[email protected]> writes:
> 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 <[email protected]>
> ---
> 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));
The only part that I didn't like, at first, was this, that the
initialization of the offload struct is divided into two parts: one to
set the command/tc, and one to set the "invalid/not set" value to all
stats fields.
I was thinking of adding a macro to do initialization of the stats
fields, but it has a problem that it won't complain when a new field is
added. Your solution should always work. I don't have better
suggestions.
Acked-by: Vinicius Costa Gomes <[email protected]>
> +
> + 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
>
--
Vinicius
Hello:
This series was applied to netdev/net-next.git (main)
by David S. Miller <[email protected]>:
On Tue, 30 May 2023 12:19:43 +0300 you wrote:
> As a result of this discussion:
> https://lore.kernel.org/intel-wired-lan/[email protected]/
>
> it became apparent that tc-taprio should make an effort to standardize
> statistics counters related to the 802.1Qbv scheduling as implemented
> by the NIC. I'm presenting here one counter suggested by the standard,
> and one counter defined by the NXP ENETC controller from LS1028A. Both
> counters are reported globally and per traffic class - drivers get
> different callbacks for reporting both of these, and get to choose what
> to report in both cases.
>
> [...]
Here is the summary with links:
- [net-next,1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()
https://git.kernel.org/netdev/net-next/c/dced11ef84fb
- [net-next,2/5] net/sched: taprio: replace tc_taprio_qopt_offload :: enable with a "cmd" enum
https://git.kernel.org/netdev/net-next/c/2d800bc500fb
- [net-next,3/5] net/sched: taprio: add netlink reporting for offload statistics counters
https://git.kernel.org/netdev/net-next/c/6c1adb650c8d
- [net-next,4/5] net: enetc: refactor enetc_setup_tc_taprio() to have a switch/case for cmd
https://git.kernel.org/netdev/net-next/c/5353599aa745
- [net-next,5/5] net: enetc: report statistics counters for taprio
https://git.kernel.org/netdev/net-next/c/4802fca8d1af
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hi Vladimir,
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Tuesday, 30 May, 2023 5:20 PM
> To: [email protected]
> Cc: David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Hadi Salim, Jamal <[email protected]>; Cong Wang
> <[email protected]>; Jiri Pirko <[email protected]>; Gomes, Vinicius
> <[email protected]>; Kurt Kanzenbach <[email protected]>;
> Gerhard Engleder <[email protected]>; Nambiar, Amritha
> <[email protected]>; Ferenc Fejes <[email protected]>;
> Xiaoliang Yang <[email protected]>; Roger Quadros
> <[email protected]>; Pranavi Somisetty <[email protected]>;
> Harini Katakam <[email protected]>; Giuseppe Cavallaro
> <[email protected]>; Alexandre Torgue
> <[email protected]>; Sit, Michael Wei Hong
> <[email protected]>; Ismail, Mohammad Athari
> <[email protected]>; Oleksij Rempel <linux@rempel-
> privat.de>; Keller, Jacob E <[email protected]>; linux-
> [email protected]; Andrew Lunn <[email protected]>; Florian Fainelli
> <[email protected]>; Claudiu Manoil <[email protected]>;
> Alexandre Belloni <[email protected]>;
> [email protected]; Brandeburg, Jesse
> <[email protected]>; Nguyen, Anthony L
> <[email protected]>; Horatiu Vultur
> <[email protected]>; Jose Abreu <[email protected]>;
> Maxime Coquelin <[email protected]>; intel-wired-
> [email protected]; Zulkifli, Muhammad Husaini
> <[email protected]>
> 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 <[email protected]>
Tested-by: Muhammad Husaini Zulkifli <[email protected]>
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
On Tue, May 30, 2023 at 03:52:17PM -0700, Vinicius Costa Gomes wrote:
> > + memset(stats, 0xff, sizeof(*stats));
>
> The only part that I didn't like, at first, was this, that the
> initialization of the offload struct is divided into two parts: one to
> set the command/tc, and one to set the "invalid/not set" value to all
> stats fields.
>
> I was thinking of adding a macro to do initialization of the stats
> fields, but it has a problem that it won't complain when a new field is
> added. Your solution should always work. I don't have better
> suggestions.
Right, it's no coincidence that it's where it is and the way it is.
Again, I drew inspiration from stats_prepare_data() in ethtool.
> Acked-by: Vinicius Costa Gomes <[email protected]>
Thanks for the review.