[Cake] [PATCH] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Cake] [PATCH] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Toke Høiland-Jørgensen]

This splits out the tin stats from tc_cake_xstats, which seems like the
least intrusive way of decreasing the size of the stats structure. This
way, we can send only the statistics corresponding to the actual number of
allocated tins, rather than having the xstats structure always be allocated
for the full number of tins.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
This patch is against the for_upstream_4.16 branch. It compiles, but is
otherwise completely untested. No idea if it will fix the build bot
error from the upstream submission, but couldn't think of any other
obvious way to deal with it...

 pkt_sched.h | 46 ++++++++++++++++++++++++++--------------------
 sch_cake.c  | 54 +++++++++++++++++++++++++++++-------------------------
 2 files changed, 55 insertions(+), 45 deletions(-)

diff --git a/pkt_sched.h b/pkt_sched.h
index ed7c111..6fecc21 100644
--- a/pkt_sched.h
+++ b/pkt_sched.h
@@ -964,33 +964,39 @@ struct tc_cake_traffic_stats {
 };
 
 #define TC_CAKE_MAX_TINS (8)
+struct tc_cake_tin_stats {
+
+	__u32 threshold_rate;
+	__u32 target_us;
+	struct tc_cake_traffic_stats sent;
+	struct tc_cake_traffic_stats dropped;
+	struct tc_cake_traffic_stats ecn_marked;
+	struct tc_cake_traffic_stats backlog;
+	__u32 interval_us;
+	__u32 way_indirect_hits;
+	__u32 way_misses;
+	__u32 way_collisions;
+	__u32 peak_delay_us; /* ~= bulk flow delay */
+	__u32 avge_delay_us;
+	__u32 base_delay_us; /* ~= sparse flows delay */
+	__u16 sparse_flows;
+	__u16 bulk_flows;
+	__u16 unresponse_flows; /* v4 - was u32 last_len */
+	__u16 spare; /* v4 - split last_len */
+	__u32 max_skblen;
+	struct tc_cake_traffic_stats ack_drops; /* v5 */
+};
+
 struct tc_cake_xstats {
 	__u16 version;  /* == 5, increments when struct extended */
 	__u8  max_tins; /* == TC_CAKE_MAX_TINS */
 	__u8  tin_cnt;  /* <= TC_CAKE_MAX_TINS */
-
-	__u32 threshold_rate[TC_CAKE_MAX_TINS];
-	__u32 target_us[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats sent[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats dropped[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats ecn_marked[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats backlog[TC_CAKE_MAX_TINS];
-	__u32 interval_us[TC_CAKE_MAX_TINS];
-	__u32 way_indirect_hits[TC_CAKE_MAX_TINS];
-	__u32 way_misses[TC_CAKE_MAX_TINS];
-	__u32 way_collisions[TC_CAKE_MAX_TINS];
-	__u32 peak_delay_us[TC_CAKE_MAX_TINS]; /* ~= bulk flow delay */
-	__u32 avge_delay_us[TC_CAKE_MAX_TINS];
-	__u32 base_delay_us[TC_CAKE_MAX_TINS]; /* ~= sparse flows delay */
-	__u16 sparse_flows[TC_CAKE_MAX_TINS];
-	__u16 bulk_flows[TC_CAKE_MAX_TINS];
-	__u16 unresponse_flows[TC_CAKE_MAX_TINS]; /* v4 - was u32 last_len */
-	__u16 spare[TC_CAKE_MAX_TINS]; /* v4 - split last_len */
-	__u32 max_skblen[TC_CAKE_MAX_TINS];
 	__u32 capacity_estimate;  /* version 2 */
 	__u32 memory_limit;       /* version 3 */
 	__u32 memory_used;	  /* version 3 */
-	struct tc_cake_traffic_stats ack_drops[TC_CAKE_MAX_TINS]; /* v5 */
+
+	struct tc_cake_tin_stats tin_stats[0]; /* keep last */
 };
 
+
 #endif
diff --git a/sch_cake.c b/sch_cake.c
index 7f6ff8e..7bdc01c 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -2478,9 +2478,12 @@ nla_put_failure:
 static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 {
 	struct cake_sched_data *q = qdisc_priv(sch);
-	struct tc_cake_xstats *st = kvzalloc(sizeof(*st), GFP_KERNEL);
+	struct tc_cake_xstats *st;
+	size_t size = sizeof(*st) + sizeof(struct tc_cake_tin_stats) * q->tin_cnt;
 	int i;
 
+	st = kvzalloc(size, GFP_KERNEL);
+
 	if (!st)
 		return -ENOMEM;
 
@@ -2490,39 +2493,40 @@ static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 
 	for (i = 0; i < q->tin_cnt; i++) {
 		struct cake_tin_data *b = &q->tins[q->tin_order[i]];
+		struct tc_cake_tin_stats *tstat = &st->tin_stats[i];
 
-		st->threshold_rate[i] = b->tin_rate_bps;
-		st->target_us[i]      = cobalt_time_to_us(b->cparams.target);
-		st->interval_us[i]    = cobalt_time_to_us(b->cparams.interval);
+		tstat->threshold_rate = b->tin_rate_bps;
+		tstat->target_us      = cobalt_time_to_us(b->cparams.target);
+		tstat->interval_us    = cobalt_time_to_us(b->cparams.interval);
 
 		/* TODO FIXME: add missing aspects of these composite stats */
-		st->sent[i].packets       = b->packets;
-		st->sent[i].bytes	  = b->bytes;
-		st->dropped[i].packets    = b->tin_dropped;
-		st->ecn_marked[i].packets = b->tin_ecn_mark;
-		st->backlog[i].bytes      = b->tin_backlog;
-		st->ack_drops[i].packets  = b->ack_drops;
-
-		st->peak_delay_us[i] = cobalt_time_to_us(b->peak_delay);
-		st->avge_delay_us[i] = cobalt_time_to_us(b->avge_delay);
-		st->base_delay_us[i] = cobalt_time_to_us(b->base_delay);
-
-		st->way_indirect_hits[i] = b->way_hits;
-		st->way_misses[i]	 = b->way_misses;
-		st->way_collisions[i]    = b->way_collisions;
-
-		st->sparse_flows[i]      = b->sparse_flow_count +
+		tstat->sent.packets       = b->packets;
+		tstat->sent.bytes	  = b->bytes;
+		tstat->dropped.packets    = b->tin_dropped;
+		tstat->ecn_marked.packets = b->tin_ecn_mark;
+		tstat->backlog.bytes      = b->tin_backlog;
+		tstat->ack_drops.packets  = b->ack_drops;
+
+		tstat->peak_delay_us = cobalt_time_to_us(b->peak_delay);
+		tstat->avge_delay_us = cobalt_time_to_us(b->avge_delay);
+		tstat->base_delay_us = cobalt_time_to_us(b->base_delay);
+
+		tstat->way_indirect_hits = b->way_hits;
+		tstat->way_misses	 = b->way_misses;
+		tstat->way_collisions    = b->way_collisions;
+
+		tstat->sparse_flows      = b->sparse_flow_count +
 					   b->decaying_flow_count;
-		st->bulk_flows[i]	 = b->bulk_flow_count;
-		st->unresponse_flows[i]  = b->unresponsive_flow_count;
-		st->spare[i]		 = 0;
-		st->max_skblen[i]	 = b->max_skblen;
+		tstat->bulk_flows	 = b->bulk_flow_count;
+		tstat->unresponse_flows  = b->unresponsive_flow_count;
+		tstat->spare		 = 0;
+		tstat->max_skblen	 = b->max_skblen;
 	}
 	st->capacity_estimate = q->avg_peak_bandwidth;
 	st->memory_limit      = q->buffer_limit;
 	st->memory_used       = q->buffer_max_used;
 
-	i = gnet_stats_copy_app(d, st, sizeof(*st));
+	i = gnet_stats_copy_app(d, st, size);
 	cake_free(st);
 	return i;
 }
-- 
2.15.1

Re: [Cake] [PATCH] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Dave Taht]

The puzzling thing about the reported error for this function is that
the static analysis checker is complaining about 1400+ bytes being
used *on the stack* (in gnet_stats_copy-app?) for the pa-risc
architecture, and that arch, only, which I just don't "get".

That said, this patch will end up copying less data most of the time,
and fool the static analysis checker in that case.

Re: [Cake] [PATCH] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Toke Høiland-Jørgensen]

On 26 December 2017 17:28:39 CET, Dave Taht <dave.taht@gmail.com> wrote:
>The puzzling thing about the reported error for this function is that
>the static analysis checker is complaining about 1400+ bytes being
>used *on the stack* (in gnet_stats_copy-app?) for the pa-risc
>architecture, and that arch, only, which I just don't "get".

Maybe the compiler is being clever and detecting a static allocation which it then moves to the stack or something? Didn't quite understand that error either...

>That said, this patch will end up copying less data most of the time,
>and fool the static analysis checker in that case.

Yup, that's what I was going for... ;)

-Toke

[Cake] [PATCH v2] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Toke Høiland-Jørgensen]

This splits out the tin stats from tc_cake_xstats, which seems like the
least intrusive way of decreasing the size of the stats structure. This
way, we can send only the statistics corresponding to the actual number of
allocated tins, rather than having the xstats structure always be allocated
for the full number of tins.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
v2:
- If we want to be able to extend the tc_cake_tin_stats at a later date
  (and if we don't the 'version' field is sort of meaningless), its size
  need to be included somewhere so userspace can skip over unknown
  fields. So add the tin stats size to struct tc_cake_xstats.

 pkt_sched.h | 50 ++++++++++++++++++++++++++++----------------------
 sch_cake.c  | 56 ++++++++++++++++++++++++++++++--------------------------
 2 files changed, 58 insertions(+), 48 deletions(-)

diff --git a/pkt_sched.h b/pkt_sched.h
index ed7c111..096642e 100644
--- a/pkt_sched.h
+++ b/pkt_sched.h
@@ -964,33 +964,39 @@ struct tc_cake_traffic_stats {
 };
 
 #define TC_CAKE_MAX_TINS (8)
+struct tc_cake_tin_stats {
+
+	__u32 threshold_rate;
+	__u32 target_us;
+	struct tc_cake_traffic_stats sent;
+	struct tc_cake_traffic_stats dropped;
+	struct tc_cake_traffic_stats ecn_marked;
+	struct tc_cake_traffic_stats backlog;
+	__u32 interval_us;
+	__u32 way_indirect_hits;
+	__u32 way_misses;
+	__u32 way_collisions;
+	__u32 peak_delay_us; /* ~= bulk flow delay */
+	__u32 avge_delay_us;
+	__u32 base_delay_us; /* ~= sparse flows delay */
+	__u16 sparse_flows;
+	__u16 bulk_flows;
+	__u16 unresponse_flows; /* v4 - was u32 last_len */
+	__u16 spare; /* v4 - split last_len */
+	__u32 max_skblen;
+	struct tc_cake_traffic_stats ack_drops; /* v5 */
+};
+
 struct tc_cake_xstats {
-	__u16 version;  /* == 5, increments when struct extended */
-	__u8  max_tins; /* == TC_CAKE_MAX_TINS */
+	__u16 tin_stats_size; /* == sizeof(struct tc_cake_tin_stats) */
 	__u8  tin_cnt;  /* <= TC_CAKE_MAX_TINS */
-
-	__u32 threshold_rate[TC_CAKE_MAX_TINS];
-	__u32 target_us[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats sent[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats dropped[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats ecn_marked[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats backlog[TC_CAKE_MAX_TINS];
-	__u32 interval_us[TC_CAKE_MAX_TINS];
-	__u32 way_indirect_hits[TC_CAKE_MAX_TINS];
-	__u32 way_misses[TC_CAKE_MAX_TINS];
-	__u32 way_collisions[TC_CAKE_MAX_TINS];
-	__u32 peak_delay_us[TC_CAKE_MAX_TINS]; /* ~= bulk flow delay */
-	__u32 avge_delay_us[TC_CAKE_MAX_TINS];
-	__u32 base_delay_us[TC_CAKE_MAX_TINS]; /* ~= sparse flows delay */
-	__u16 sparse_flows[TC_CAKE_MAX_TINS];
-	__u16 bulk_flows[TC_CAKE_MAX_TINS];
-	__u16 unresponse_flows[TC_CAKE_MAX_TINS]; /* v4 - was u32 last_len */
-	__u16 spare[TC_CAKE_MAX_TINS]; /* v4 - split last_len */
-	__u32 max_skblen[TC_CAKE_MAX_TINS];
+	__u8  version;  /* == 5, increments when struct extended */
 	__u32 capacity_estimate;  /* version 2 */
 	__u32 memory_limit;       /* version 3 */
 	__u32 memory_used;	  /* version 3 */
-	struct tc_cake_traffic_stats ack_drops[TC_CAKE_MAX_TINS]; /* v5 */
+
+	struct tc_cake_tin_stats tin_stats[0]; /* keep last */
 };
 
+
 #endif
diff --git a/sch_cake.c b/sch_cake.c
index 7f6ff8e..ba316a7 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -2478,51 +2478,55 @@ nla_put_failure:
 static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 {
 	struct cake_sched_data *q = qdisc_priv(sch);
-	struct tc_cake_xstats *st = kvzalloc(sizeof(*st), GFP_KERNEL);
+	struct tc_cake_xstats *st;
+	size_t size = sizeof(*st) + sizeof(struct tc_cake_tin_stats) * q->tin_cnt;
 	int i;
 
+	st = kvzalloc(size, GFP_KERNEL);
+
 	if (!st)
 		return -ENOMEM;
 
 	st->version = 5;
-	st->max_tins = TC_CAKE_MAX_TINS;
+	st->tin_stats_size = sizeof(struct tc_cake_tin_stats);
 	st->tin_cnt = q->tin_cnt;
 
 	for (i = 0; i < q->tin_cnt; i++) {
 		struct cake_tin_data *b = &q->tins[q->tin_order[i]];
+		struct tc_cake_tin_stats *tstat = &st->tin_stats[i];
 
-		st->threshold_rate[i] = b->tin_rate_bps;
-		st->target_us[i]      = cobalt_time_to_us(b->cparams.target);
-		st->interval_us[i]    = cobalt_time_to_us(b->cparams.interval);
+		tstat->threshold_rate = b->tin_rate_bps;
+		tstat->target_us      = cobalt_time_to_us(b->cparams.target);
+		tstat->interval_us    = cobalt_time_to_us(b->cparams.interval);
 
 		/* TODO FIXME: add missing aspects of these composite stats */
-		st->sent[i].packets       = b->packets;
-		st->sent[i].bytes	  = b->bytes;
-		st->dropped[i].packets    = b->tin_dropped;
-		st->ecn_marked[i].packets = b->tin_ecn_mark;
-		st->backlog[i].bytes      = b->tin_backlog;
-		st->ack_drops[i].packets  = b->ack_drops;
-
-		st->peak_delay_us[i] = cobalt_time_to_us(b->peak_delay);
-		st->avge_delay_us[i] = cobalt_time_to_us(b->avge_delay);
-		st->base_delay_us[i] = cobalt_time_to_us(b->base_delay);
-
-		st->way_indirect_hits[i] = b->way_hits;
-		st->way_misses[i]	 = b->way_misses;
-		st->way_collisions[i]    = b->way_collisions;
-
-		st->sparse_flows[i]      = b->sparse_flow_count +
+		tstat->sent.packets       = b->packets;
+		tstat->sent.bytes	  = b->bytes;
+		tstat->dropped.packets    = b->tin_dropped;
+		tstat->ecn_marked.packets = b->tin_ecn_mark;
+		tstat->backlog.bytes      = b->tin_backlog;
+		tstat->ack_drops.packets  = b->ack_drops;
+
+		tstat->peak_delay_us = cobalt_time_to_us(b->peak_delay);
+		tstat->avge_delay_us = cobalt_time_to_us(b->avge_delay);
+		tstat->base_delay_us = cobalt_time_to_us(b->base_delay);
+
+		tstat->way_indirect_hits = b->way_hits;
+		tstat->way_misses	 = b->way_misses;
+		tstat->way_collisions    = b->way_collisions;
+
+		tstat->sparse_flows      = b->sparse_flow_count +
 					   b->decaying_flow_count;
-		st->bulk_flows[i]	 = b->bulk_flow_count;
-		st->unresponse_flows[i]  = b->unresponsive_flow_count;
-		st->spare[i]		 = 0;
-		st->max_skblen[i]	 = b->max_skblen;
+		tstat->bulk_flows	 = b->bulk_flow_count;
+		tstat->unresponse_flows  = b->unresponsive_flow_count;
+		tstat->spare		 = 0;
+		tstat->max_skblen	 = b->max_skblen;
 	}
 	st->capacity_estimate = q->avg_peak_bandwidth;
 	st->memory_limit      = q->buffer_limit;
 	st->memory_used       = q->buffer_max_used;
 
-	i = gnet_stats_copy_app(d, st, sizeof(*st));
+	i = gnet_stats_copy_app(d, st, size);
 	cake_free(st);
 	return i;
 }
-- 
2.16.1

Re: [Cake] [PATCH v2] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Jonathan Morton]

> On 27 Jan, 2018, at 3:05 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> This splits out the tin stats from tc_cake_xstats, which seems like the
> least intrusive way of decreasing the size of the stats structure. This
> way, we can send only the statistics corresponding to the actual number of
> allocated tins, rather than having the xstats structure always be allocated
> for the full number of tins.

In order for this patch to work, we would need a corresponding patch for iproute2 so that it can successfully read the new structure - including correcting for changes in the size field.

 - Jonathan Morton

Re: [Cake] [PATCH v2] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 27 Jan, 2018, at 3:05 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> This splits out the tin stats from tc_cake_xstats, which seems like the
>> least intrusive way of decreasing the size of the stats structure. This
>> way, we can send only the statistics corresponding to the actual number of
>> allocated tins, rather than having the xstats structure always be allocated
>> for the full number of tins.
>
> In order for this patch to work, we would need a corresponding patch
> for iproute2 so that it can successfully read the new structure -
> including correcting for changes in the size field.

Yup, that's on my list; haven't gotten around to it yet... :)

-Toke

[Cake] [PATCH v3] Split tin stats to its own structure to decrease size of tc_cake_xstats

[Toke Høiland-Jørgensen]

This splits out the tin stats from tc_cake_xstats, which seems like the
least intrusive way of decreasing the size of the stats structure. This
way, we can send only the statistics corresponding to the actual number of
allocated tins, rather than having the xstats structure always be allocated
for the full number of tins.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
Updated in v3:
- Keep the version field in place in the xstats structure, and set it to
  a value higher than 0xFF, which will make old versions of tc ignore
  it. This means old versions of tc simply won't print statistics,
  rather than print garbled statistics because the struct layout is
  wrong.
- Actually tested the code :)

 pkt_sched.h | 52 +++++++++++++++++++++++++++++-----------------------
 sch_cake.c  | 58 +++++++++++++++++++++++++++++++---------------------------
 2 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/pkt_sched.h b/pkt_sched.h
index ed7c111..3a86d60 100644
--- a/pkt_sched.h
+++ b/pkt_sched.h
@@ -964,33 +964,39 @@ struct tc_cake_traffic_stats {
 };
 
 #define TC_CAKE_MAX_TINS (8)
-struct tc_cake_xstats {
-	__u16 version;  /* == 5, increments when struct extended */
-	__u8  max_tins; /* == TC_CAKE_MAX_TINS */
-	__u8  tin_cnt;  /* <= TC_CAKE_MAX_TINS */
+struct tc_cake_tin_stats {
+
+	__u32 threshold_rate;
+	__u32 target_us;
+	struct tc_cake_traffic_stats sent;
+	struct tc_cake_traffic_stats dropped;
+	struct tc_cake_traffic_stats ecn_marked;
+	struct tc_cake_traffic_stats backlog;
+	__u32 interval_us;
+	__u32 way_indirect_hits;
+	__u32 way_misses;
+	__u32 way_collisions;
+	__u32 peak_delay_us; /* ~= bulk flow delay */
+	__u32 avge_delay_us;
+	__u32 base_delay_us; /* ~= sparse flows delay */
+	__u16 sparse_flows;
+	__u16 bulk_flows;
+	__u16 unresponse_flows; /* v4 - was u32 last_len */
+	__u16 spare; /* v4 - split last_len */
+	__u32 max_skblen;
+	struct tc_cake_traffic_stats ack_drops; /* v5 */
+};
 
-	__u32 threshold_rate[TC_CAKE_MAX_TINS];
-	__u32 target_us[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats sent[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats dropped[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats ecn_marked[TC_CAKE_MAX_TINS];
-	struct tc_cake_traffic_stats backlog[TC_CAKE_MAX_TINS];
-	__u32 interval_us[TC_CAKE_MAX_TINS];
-	__u32 way_indirect_hits[TC_CAKE_MAX_TINS];
-	__u32 way_misses[TC_CAKE_MAX_TINS];
-	__u32 way_collisions[TC_CAKE_MAX_TINS];
-	__u32 peak_delay_us[TC_CAKE_MAX_TINS]; /* ~= bulk flow delay */
-	__u32 avge_delay_us[TC_CAKE_MAX_TINS];
-	__u32 base_delay_us[TC_CAKE_MAX_TINS]; /* ~= sparse flows delay */
-	__u16 sparse_flows[TC_CAKE_MAX_TINS];
-	__u16 bulk_flows[TC_CAKE_MAX_TINS];
-	__u16 unresponse_flows[TC_CAKE_MAX_TINS]; /* v4 - was u32 last_len */
-	__u16 spare[TC_CAKE_MAX_TINS]; /* v4 - split last_len */
-	__u32 max_skblen[TC_CAKE_MAX_TINS];
+struct tc_cake_xstats {
+	__u16 version;
+	__u16 tin_stats_size; /* == sizeof(struct tc_cake_tin_stats) */
 	__u32 capacity_estimate;  /* version 2 */
 	__u32 memory_limit;       /* version 3 */
 	__u32 memory_used;	  /* version 3 */
-	struct tc_cake_traffic_stats ack_drops[TC_CAKE_MAX_TINS]; /* v5 */
+	__u8  tin_cnt;  /* <= TC_CAKE_MAX_TINS */
+
+	struct tc_cake_tin_stats tin_stats[0]; /* keep last */
 };
 
+
 #endif
diff --git a/sch_cake.c b/sch_cake.c
index 7f6ff8e..62b67e7 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -2478,51 +2478,55 @@ nla_put_failure:
 static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 {
 	struct cake_sched_data *q = qdisc_priv(sch);
-	struct tc_cake_xstats *st = kvzalloc(sizeof(*st), GFP_KERNEL);
+	struct tc_cake_xstats *st;
+	size_t size = sizeof(*st) + sizeof(struct tc_cake_tin_stats) * q->tin_cnt;
 	int i;
 
+	st = kvzalloc(size, GFP_KERNEL);
+
 	if (!st)
 		return -ENOMEM;
 
-	st->version = 5;
-	st->max_tins = TC_CAKE_MAX_TINS;
+	st->version = 0xFF + 1; /* old userspace code discards versions > 0xFF */
+	st->tin_stats_size = sizeof(struct tc_cake_tin_stats);
 	st->tin_cnt = q->tin_cnt;
 
 	for (i = 0; i < q->tin_cnt; i++) {
 		struct cake_tin_data *b = &q->tins[q->tin_order[i]];
+		struct tc_cake_tin_stats *tstat = &st->tin_stats[i];
 
-		st->threshold_rate[i] = b->tin_rate_bps;
-		st->target_us[i]      = cobalt_time_to_us(b->cparams.target);
-		st->interval_us[i]    = cobalt_time_to_us(b->cparams.interval);
+		tstat->threshold_rate = b->tin_rate_bps;
+		tstat->target_us      = cobalt_time_to_us(b->cparams.target);
+		tstat->interval_us    = cobalt_time_to_us(b->cparams.interval);
 
 		/* TODO FIXME: add missing aspects of these composite stats */
-		st->sent[i].packets       = b->packets;
-		st->sent[i].bytes	  = b->bytes;
-		st->dropped[i].packets    = b->tin_dropped;
-		st->ecn_marked[i].packets = b->tin_ecn_mark;
-		st->backlog[i].bytes      = b->tin_backlog;
-		st->ack_drops[i].packets  = b->ack_drops;
-
-		st->peak_delay_us[i] = cobalt_time_to_us(b->peak_delay);
-		st->avge_delay_us[i] = cobalt_time_to_us(b->avge_delay);
-		st->base_delay_us[i] = cobalt_time_to_us(b->base_delay);
-
-		st->way_indirect_hits[i] = b->way_hits;
-		st->way_misses[i]	 = b->way_misses;
-		st->way_collisions[i]    = b->way_collisions;
-
-		st->sparse_flows[i]      = b->sparse_flow_count +
+		tstat->sent.packets       = b->packets;
+		tstat->sent.bytes	  = b->bytes;
+		tstat->dropped.packets    = b->tin_dropped;
+		tstat->ecn_marked.packets = b->tin_ecn_mark;
+		tstat->backlog.bytes      = b->tin_backlog;
+		tstat->ack_drops.packets  = b->ack_drops;
+
+		tstat->peak_delay_us = cobalt_time_to_us(b->peak_delay);
+		tstat->avge_delay_us = cobalt_time_to_us(b->avge_delay);
+		tstat->base_delay_us = cobalt_time_to_us(b->base_delay);
+
+		tstat->way_indirect_hits = b->way_hits;
+		tstat->way_misses	 = b->way_misses;
+		tstat->way_collisions    = b->way_collisions;
+
+		tstat->sparse_flows      = b->sparse_flow_count +
 					   b->decaying_flow_count;
-		st->bulk_flows[i]	 = b->bulk_flow_count;
-		st->unresponse_flows[i]  = b->unresponsive_flow_count;
-		st->spare[i]		 = 0;
-		st->max_skblen[i]	 = b->max_skblen;
+		tstat->bulk_flows	 = b->bulk_flow_count;
+		tstat->unresponse_flows  = b->unresponsive_flow_count;
+		tstat->spare		 = 0;
+		tstat->max_skblen	 = b->max_skblen;
 	}
 	st->capacity_estimate = q->avg_peak_bandwidth;
 	st->memory_limit      = q->buffer_limit;
 	st->memory_used       = q->buffer_max_used;
 
-	i = gnet_stats_copy_app(d, st, sizeof(*st));
+	i = gnet_stats_copy_app(d, st, size);
 	cake_free(st);
 	return i;
 }
-- 
2.16.1

[Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

This updates tc to understand the updated cake xstats structure (which
splits out the tin stats in a separate structure the length of which is
included in the containing struct).

Old versions of the cake stats will no longer be understood by the
resulting version of tc.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 tc/q_cake.c | 103 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 51 insertions(+), 52 deletions(-)

diff --git a/tc/q_cake.c b/tc/q_cake.c
index 6987c4d..3ddc13c 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -555,6 +555,11 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	return 0;
 }
 
+#define FOR_EACH_TIN(xstats, tst, i)				\
+	for(tst = xstats->tin_stats, i = 0;			\
+	i < xstats->tin_cnt;						\
+	    i++, tst = ((void *) xstats->tin_stats) + xstats->tin_stats_size * i)
+
 static int cake_print_xstats(struct qdisc_util *qu, FILE *f,
 				 struct rtattr *xstats)
 {
@@ -597,17 +602,15 @@ static int cake_print_xstats(struct qdisc_util *qu, FILE *f,
 				fprintf(f, " drop_next %s",
 					sprint_time(st->class_stats.drop_next, b1));
 		}
-	} else if (stnc->version >= 1 && stnc->version < 0xFF
-				&& stnc->max_tins == TC_CAKE_MAX_TINS
-				&& RTA_PAYLOAD(xstats) >= offsetof(struct tc_cake_xstats, capacity_estimate))
+	} else if (stnc->version > 0xFF
+		&& RTA_PAYLOAD(xstats) >= (sizeof(struct tc_cake_xstats) +
+					stnc->tin_stats_size * stnc->tin_cnt))
 	{
+		struct tc_cake_tin_stats  *tst;
 		int i;
 
-		if(stnc->version >= 3)
-			fprintf(f, " memory used: %s of %s\n", sprint_size(stnc->memory_used, b1), sprint_size(stnc->memory_limit, b2));
-
-		if(stnc->version >= 2)
-			fprintf(f, " capacity estimate: %s\n", sprint_rate(stnc->capacity_estimate, b1));
+		fprintf(f, " memory used: %s of %s\n", sprint_size(stnc->memory_used, b1), sprint_size(stnc->memory_limit, b2));
+		fprintf(f, " capacity estimate: %s\n", sprint_rate(stnc->capacity_estimate, b1));
 
 		switch(stnc->tin_cnt) {
 		case 3:
@@ -630,97 +633,93 @@ static int cake_print_xstats(struct qdisc_util *qu, FILE *f,
 		};
 
 		fprintf(f, "  thresh  ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12s", sprint_rate(stnc->threshold_rate[i], b1));
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12s", sprint_rate(tst->threshold_rate, b1));
 		fprintf(f, "\n");
 
 		fprintf(f, "  target  ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12s", sprint_time(stnc->target_us[i], b1));
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12s", sprint_time(tst->target_us, b1));
 		fprintf(f, "\n");
 
 		fprintf(f, "  interval");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12s", sprint_time(stnc->interval_us[i], b1));
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12s", sprint_time(tst->interval_us, b1));
 		fprintf(f, "\n");
 
 		fprintf(f, "  pk_delay");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12s", sprint_time(stnc->peak_delay_us[i], b1));
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12s", sprint_time(tst->peak_delay_us, b1));
 		fprintf(f, "\n");
 
 		fprintf(f, "  av_delay");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12s", sprint_time(stnc->avge_delay_us[i], b1));
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12s", sprint_time(tst->avge_delay_us, b1));
 		fprintf(f, "\n");
 
 		fprintf(f, "  sp_delay");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12s", sprint_time(stnc->base_delay_us[i], b1));
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12s", sprint_time(tst->base_delay_us, b1));
 		fprintf(f, "\n");
 
 		fprintf(f, "  pkts    ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->sent[i].packets);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->sent.packets);
 		fprintf(f, "\n");
 
 		fprintf(f, "  bytes   ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12llu", stnc->sent[i].bytes);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12llu", tst->sent.bytes);
 		fprintf(f, "\n");
 
 		fprintf(f, "  way_inds");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->way_indirect_hits[i]);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->way_indirect_hits);
 		fprintf(f, "\n");
 
 		fprintf(f, "  way_miss");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->way_misses[i]);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->way_misses);
 		fprintf(f, "\n");
 
 		fprintf(f, "  way_cols");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->way_collisions[i]);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->way_collisions);
 		fprintf(f, "\n");
 
 		fprintf(f, "  drops   ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->dropped[i].packets);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->dropped.packets);
 		fprintf(f, "\n");
 
 		fprintf(f, "  marks   ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->ecn_marked[i].packets);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->ecn_marked.packets);
 		fprintf(f, "\n");
 
-		if(stnc->version >= 5) {
-			fprintf(f, "  ack_drop");
-			for(i=0; i < stnc->tin_cnt; i++)
-				fprintf(f, "%12u", stnc->ack_drops[i].packets);
-			fprintf(f, "\n");
-		}
+		fprintf(f, "  ack_drop");
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->ack_drops.packets);
+		fprintf(f, "\n");
 
 		fprintf(f, "  sp_flows");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->sparse_flows[i]);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->sparse_flows);
 		fprintf(f, "\n");
 
 		fprintf(f, "  bk_flows");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->bulk_flows[i]);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->bulk_flows);
 		fprintf(f, "\n");
 
-		if(stnc->version >= 4) {
-			fprintf(f, "  un_flows");
-			for(i=0; i < stnc->tin_cnt; i++)
-				fprintf(f, "%12u", stnc->unresponse_flows[i]);
-			fprintf(f, "\n");
-		}
+		fprintf(f, "  un_flows");
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->unresponse_flows);
+		fprintf(f, "\n");
 
 		fprintf(f, "  max_len ");
-		for(i=0; i < stnc->tin_cnt; i++)
-			fprintf(f, "%12u", stnc->max_skblen[i]);
+		FOR_EACH_TIN(stnc, tst, i)
+			fprintf(f, "%12u", tst->max_skblen);
 		fprintf(f, "\n");
 	} else {
 		return -1;
-- 
2.16.1

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 11 Feb, 2018, at 7:26 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> This updates tc to understand the updated cake xstats structure (which
> splits out the tin stats in a separate structure the length of which is
> included in the containing struct).
> 
> Old versions of the cake stats will no longer be understood by the
> resulting version of tc.

I think you could make a pull request for these patches now.  I can then add new stats to the new code instead of duplicating work.

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Sebastian Moeller]

Dear All, 

I am a bit confused now which tc repository is the one to track.

I believe Jonathan made changes to https://github.com/dtaht/tc-adv

while https://github.com/dtaht/iproute2-cake-next seems overall more recent (modulo Jonathan's changes)

So which one is it?

BTW, my testing so far with the latest tc-adv did not result in any crashes, but I also did not research whether the overhead account behaves like expected...

Best Regards
	Sebastian


> On Mar 1, 2018, at 12:02, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 11 Feb, 2018, at 7:26 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> This updates tc to understand the updated cake xstats structure (which
>> splits out the tin stats in a separate structure the length of which is
>> included in the containing struct).
>> 
>> Old versions of the cake stats will no longer be understood by the
>> resulting version of tc.
> 
> I think you could make a pull request for these patches now.  I can then add new stats to the new code instead of duplicating work.
> 
> - Jonathan Morton
> 
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Sebastian Moeller <moeller0@gmx.de> writes:

> Dear All, 
>
> I am a bit confused now which tc repository is the one to track.
>
> I believe Jonathan made changes to https://github.com/dtaht/tc-adv
>
> while https://github.com/dtaht/iproute2-cake-next seems overall more recent (modulo Jonathan's changes)
>
> So which one is it?
>
> BTW, my testing so far with the latest tc-adv did not result in any
> crashes, but I also did not research whether the overhead account
> behaves like expected...

Well, the tc-adv repo is the only one I have push access to at least... :)

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 1 Mar, 2018, at 1:06 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> BTW, my testing so far with the latest tc-adv did not result in any crashes, but I also did not research whether the overhead account behaves like expected...

I've just added the relevant stats output.  This breaks the stats struct again (better to do that just after breaking it once, I suppose), but I left in some padding to reduce the need for that in future.  These stats are kept globally, so are only reported as if for one tin.

So far it looks like everything is working fine and behaving as it should (on a plain Ethernet interface).  The one wrinkle is that it takes a few thousand packets for the avg_off value to converge on the true value, but that should easily be tolerable in most cases.

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 1 Mar, 2018, at 1:06 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
>> 
>> BTW, my testing so far with the latest tc-adv did not result in any crashes, but I also did not research whether the overhead account behaves like expected...
>
> I've just added the relevant stats output.  This breaks the stats
> struct again (better to do that just after breaking it once, I
> suppose), but I left in some padding to reduce the need for that in
> future.  These stats are kept globally, so are only reported as if for
> one tin.

Ah yes, the new layout makes it somewhat difficult to add new global
stats. I suppose one could store an offset in the struct as well, but
that would probably be overdoing it, methinks. A few bytes of padding
should work fine; we probably shouldn't keep adding stats forever anyway
(famous last words) :)

> So far it looks like everything is working fine and behaving as it
> should (on a plain Ethernet interface).  The one wrinkle is that it
> takes a few thousand packets for the avg_off value to converge on the
> true value, but that should easily be tolerable in most cases.

So, erm, what do the new stats mean? ;)

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 6 Mar, 2018, at 1:17 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
>> So far it looks like everything is working fine and behaving as it
>> should (on a plain Ethernet interface).  The one wrinkle is that it
>> takes a few thousand packets for the avg_off value to converge on the
>> true value, but that should easily be tolerable in most cases.
> 
> So, erm, what do the new stats mean? ;)

avg_off: the average observed offset of the transport header in packets.  This should converge to 14 on Ethernet.

max/min_tran: tracking the transport-layer size of packets; max_tran = max_len-avg_off if GSO is off.

max/min_adj: tracking the overhead-adjusted size of packets; these should be the adjusted sizes of the corresponding transport-layer sizes, so can be used to check the calculations.

It's probably feasible to lay these out better in the output, so they take up fewer lines on screen.

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Sebastian Moeller]

Hi Jonathan,



> On Mar 6, 2018, at 12:46, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 6 Mar, 2018, at 1:17 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>>> So far it looks like everything is working fine and behaving as it
>>> should (on a plain Ethernet interface).  The one wrinkle is that it
>>> takes a few thousand packets for the avg_off value to converge on the
>>> true value, but that should easily be tolerable in most cases.
>> 
>> So, erm, what do the new stats mean? ;)
> 
> avg_off: the average observed offset of the transport header in packets.  This should converge to 14 on Ethernet.
> 
> max/min_tran: tracking the transport-layer size of packets; max_tran = max_len-avg_off if GSO is off.
> 
> max/min_adj: tracking the overhead-adjusted size of packets; these should be the adjusted sizes of the corresponding transport-layer sizes, so can be used to check the calculations.
> 
> It's probably feasible to lay these out better in the output, so they take up fewer lines on screen.

Great this matches what I deduced from the numbers. I note that by replacing a cake instance with itself (so not changing the configuration) will reset the counters.
Question: am I right to assume that the _adj values are what you pass in as the true packet size into cake's calculation?
I really like these outputs so far (running flent I saw:

qdisc cake 8003: dev eth0 root refcnt 6 unlimited diffserv3 triple-isolate rtt 100.0ms noatm overhead 34 mpu 64 
 Sent 269164303 bytes 611190 pkt (dropped 0, overlimits 0 requeues 2) 
 backlog 0b 0p requeues 2 
 memory used: 10880b of 15140Kb
 capacity estimate: 0bit
                 Bulk   Best Effort      Voice
  thresh          0bit        0bit        0bit
  target         5.0ms       5.0ms       5.0ms
  interval     100.0ms     100.0ms     100.0ms
  pk_delay        14us        11us         8us
  av_delay         2us         2us         1us
  sp_delay         0us         0us         0us
  pkts           61400      395326      154464
  bytes       17617151   183250552    68296600
  way_inds           0           0           0
  way_miss           3          89          11
  way_cols           0           0           0
  drops              0           0           0
  marks              0           0           0
  ack_drop           0           0           0
  sp_flows           2          13           7
  bk_flows           0           0           0
  un_flows           0           0           0
  max_len         3012        3028        3012
  max_tran        1492
  max_adj         1526
  min_tran          28
  min_adj           64
  avg_off           14


Which is pretty much what I expected (speedtest ran over an effective MTU1492 PPPoE link*), but I still want to make more in-depth tests. Now we just need to carry this nto lede to get more testers. Plus it might make sense to merge tc-adv with iproute2-cake-next so that we have one master repository for the tc changes as well. I believe that before the xstats and Jonathan's recent updates iproute2-cake-next  was more up to date, but I am probably wrong...

Best Regards
	Sebastian

*) Interestingly takling stats later revealed:
qdisc cake 8003: dev eth0 root refcnt 6 unlimited diffserv3 triple-isolate rtt 100.0ms noatm overhead 34 mpu 64 
 Sent 743833445 bytes 1965042 pkt (dropped 0, overlimits 0 requeues 6) 
 backlog 0b 0p requeues 6 
 memory used: 16346b of 15140Kb
 capacity estimate: 0bit
                 Bulk   Best Effort      Voice
  thresh          0bit        0bit        0bit
  target         5.0ms       5.0ms       5.0ms
  interval     100.0ms     100.0ms     100.0ms
  pk_delay         8us        12us         9us
  av_delay         1us         3us         3us
  sp_delay         1us         1us         1us
  pkts          167869     1382188      414985
  bytes       49022886   511662935   183147624
  way_inds           0         341           0
  way_miss           6        1137          20
  way_cols           0           0           0
  drops              0           0           0
  marks              0           0           0
  ack_drop           0           0           0
  sp_flows           1           1           0
  bk_flows           0           0           1
  un_flows           0           0           0
  max_len         3012        3028        3012
  max_tran        1500
  max_adj         1534
  min_tran          28
  min_adj           64
  avg_off           14

Son between this sample and the speedtest the host exchanged MTU1500 packets with the local network...

> 
> - Jonathan Morton
> 

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 6 Mar, 2018, at 2:10 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
> 
> Question: am I right to assume that the _adj values are what you pass in as the true packet size into cake's calculation?

Yes, exactly.  They include ATM, PTM and MPU factors when relevant.

> I believe that before the xstats and Jonathan's recent updates iproute2-cake-next  was more up to date, but I am probably wrong...

I understand it's based on a newer version of iproute2, so would be easier to merge upstream, but its Cake support was exactly equivalent.  The latest batch of changes should be straightforward to port forward to it.

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Sebastian Moeller]

Hi Jonathan,



> On Mar 6, 2018, at 14:08, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 6 Mar, 2018, at 2:10 pm, Sebastian Moeller <moeller0@gmx.de> wrote:
>> 
>> Question: am I right to assume that the _adj values are what you pass in as the true packet size into cake's calculation?
> 
> Yes, exactly.  They include ATM, PTM and MPU factors when relevant.

Excellent, this is basically the information we would also see when tc stab is used, and being the actually input in the (maybe indirect) transfer time calculations is the best number IMHO for sanity checking... 


> 
>> I believe that before the xstats and Jonathan's recent updates iproute2-cake-next  was more up to date, but I am probably wrong...
> 
> I understand it's based on a newer version of iproute2, so would be easier to merge upstream, but its Cake support was exactly equivalent.  The latest batch of changes should be straightforward to port forward to it.

	Again, that sounds great, in your opinion now with the overhead/size accounting put on new footing and Toke's xstats work are we closer to being mergeable upstream? Any other blockers pending improvements?


Best Regards
	Sebastian

> 
> - Jonathan Morton
> 

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 11 Feb, 2018, at 7:26 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> This updates tc to understand the updated cake xstats structure (which
>> splits out the tin stats in a separate structure the length of which is
>> included in the containing struct).
>> 
>> Old versions of the cake stats will no longer be understood by the
>> resulting version of tc.
>
> I think you could make a pull request for these patches now.  I can
> then add new stats to the new code instead of duplicating work.

I think I actually have write access to the repos, so I'll just push the
commits. Sometime tonight; the code is on my laptop :)

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Jonathan Morton <chromatix99@gmail.com> writes:
>
>>> On 11 Feb, 2018, at 7:26 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> This updates tc to understand the updated cake xstats structure (which
>>> splits out the tin stats in a separate structure the length of which is
>>> included in the containing struct).
>>> 
>>> Old versions of the cake stats will no longer be understood by the
>>> resulting version of tc.
>>
>> I think you could make a pull request for these patches now.  I can
>> then add new stats to the new code instead of duplicating work.
>
> I think I actually have write access to the repos, so I'll just push the
> commits. Sometime tonight; the code is on my laptop :)

And totally forgot about it, of course. Sorry about that, pushed now :)

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Stephen Hemminger]

On Sun, 11 Feb 2018 18:26:18 +0100
Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> This updates tc to understand the updated cake xstats structure (which
> splits out the tin stats in a separate structure the length of which is
> included in the containing struct).
> 
> Old versions of the cake stats will no longer be understood by the
> resulting version of tc.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

Please consider using new json printing routines in latest iproute2.
I am not going to accept any new code upstream that doesn't support JSON output
as an option.

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Sun, 11 Feb 2018 18:26:18 +0100
> Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
>> This updates tc to understand the updated cake xstats structure (which
>> splits out the tin stats in a separate structure the length of which is
>> included in the containing struct).
>> 
>> Old versions of the cake stats will no longer be understood by the
>> resulting version of tc.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> Please consider using new json printing routines in latest iproute2.

Yeah, was going to look into that...

> I am not going to accept any new code upstream that doesn't support
> JSON output as an option.

Excellent policy! :)

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Stephen Hemminger <stephen@networkplumber.org> writes:
>
>> On Sun, 11 Feb 2018 18:26:18 +0100
>> Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>>> This updates tc to understand the updated cake xstats structure (which
>>> splits out the tin stats in a separate structure the length of which is
>>> included in the containing struct).
>>> 
>>> Old versions of the cake stats will no longer be understood by the
>>> resulting version of tc.
>>> 
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>
>> Please consider using new json printing routines in latest iproute2.
>
> Yeah, was going to look into that...

Right, so I merged the upstream iproute2-next repository into the master
branch and added JSON support to the Cake qdisc code.

I also removed a bunch of old compatibility code along the way, as well
as the fq_pie code and any changes to the repo that were not in upstream
and were not cake-related. So the delta between the tc-adv repo and
upstream is now only cake.

This was quite a bit of rearranging, so please do check if I broke
something along the way.

Other than that, though, this means that on the iproute2 side the only
thing missing before we can attempt an upstream submission is an update
of the man page, as far as I can tell. Any volunteers to do that? :)

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 6 Mar, 2018, at 11:06 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> ...on the iproute2 side the only
> thing missing before we can attempt an upstream submission is an update
> of the man page, as far as I can tell. Any volunteers to do that? :)

I could look into that tomorrow.

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 6 Mar, 2018, at 11:06 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> ...on the iproute2 side the only
>> thing missing before we can attempt an upstream submission is an update
>> of the man page, as far as I can tell. Any volunteers to do that? :)
>
> I could look into that tomorrow.

Awesome! Is there anything else you want to add on the kernel side
before we submit?

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

Hi All,



> On 7 Mar 2018, at 08:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Jonathan Morton <chromatix99@gmail.com> writes:
> 
>>> On 6 Mar, 2018, at 11:06 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> ...on the iproute2 side the only
>>> thing missing before we can attempt an upstream submission is an update
>>> of the man page, as far as I can tell. Any volunteers to do that? :)
>> 
>> I could look into that tomorrow.
> 
> Awesome! Is there anything else you want to add on the kernel side
> before we submit?

I think there was mention of the repo that Dave started on the last round of getting cake in upstream iproute2.  There were a few man page tweaks & output formatting tweaks which ideally shouldn’t be lost.  I’ve forked that fork on github here https://github.com/ldir-EDB0/iproute2-cake-next/tree/cake  rebased onto release tag v4.15.0 and pulled in the the recent changes (overhead handling paradigm, xstats & the JSON output).

In theory I guess there should be no/little difference to tc-adv/master now but….

Anyway, it’s there if it’s of any interest… it’ll be the basis of what gets sent into LEDE a bit later today.


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> Hi All,
>
>
>
>> On 7 Mar 2018, at 08:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Jonathan Morton <chromatix99@gmail.com> writes:
>> 
>>>> On 6 Mar, 2018, at 11:06 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>> 
>>>> ...on the iproute2 side the only
>>>> thing missing before we can attempt an upstream submission is an update
>>>> of the man page, as far as I can tell. Any volunteers to do that? :)
>>> 
>>> I could look into that tomorrow.
>> 
>> Awesome! Is there anything else you want to add on the kernel side
>> before we submit?
>
> I think there was mention of the repo that Dave started on the last
> round of getting cake in upstream iproute2. There were a few man page
> tweaks & output formatting tweaks which ideally shouldn’t be lost.
> I’ve forked that fork on github here
> https://github.com/ldir-EDB0/iproute2-cake-next/tree/cake rebased onto
> release tag v4.15.0 and pulled in the the recent changes (overhead
> handling paradigm, xstats & the JSON output).
>
> In theory I guess there should be no/little difference to tc-adv/master now but….
>
> Anyway, it’s there if it’s of any interest… it’ll be the basis of what
> gets sent into LEDE a bit later today.

Please don't put something different into LEDE than what we're working
on upstreaming. It is difficult enough to keep track of the different
versions as they are. The tc-adv repo is already rebased on the upstream
iproute2-next, so if there is anything else that needs to be changed, it
should be changed in that repo and not in a fork...

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>
>> Hi All,
>>
>>
>>
>>> On 7 Mar 2018, at 08:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> Jonathan Morton <chromatix99@gmail.com> writes:
>>> 
>>>>> On 6 Mar, 2018, at 11:06 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>>> 
>>>>> ...on the iproute2 side the only
>>>>> thing missing before we can attempt an upstream submission is an update
>>>>> of the man page, as far as I can tell. Any volunteers to do that? :)
>>>> 
>>>> I could look into that tomorrow.
>>> 
>>> Awesome! Is there anything else you want to add on the kernel side
>>> before we submit?
>>
>> I think there was mention of the repo that Dave started on the last
>> round of getting cake in upstream iproute2. There were a few man page
>> tweaks & output formatting tweaks which ideally shouldn’t be lost.
>> I’ve forked that fork on github here
>> https://github.com/ldir-EDB0/iproute2-cake-next/tree/cake rebased onto
>> release tag v4.15.0 and pulled in the the recent changes (overhead
>> handling paradigm, xstats & the JSON output).
>>
>> In theory I guess there should be no/little difference to tc-adv/master now but….
>>
>> Anyway, it’s there if it’s of any interest… it’ll be the basis of what
>> gets sent into LEDE a bit later today.
>
> Please don't put something different into LEDE than what we're working
> on upstreaming. It is difficult enough to keep track of the different
> versions as they are. The tc-adv repo is already rebased on the upstream
> iproute2-next, so if there is anything else that needs to be changed, it
> should be changed in that repo and not in a fork...

I pulled in the man page changes from Dave's iproute2-next fork to the
tc-adv repo now...

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

There were some useful stats column re-alignment changes as well, wonder if you got those?

> On 7 Mar 2018, at 10:36, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
> 
>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>> 
>>> Hi All,
>>> 
>>> 
>>> 
>>>> On 7 Mar 2018, at 08:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>> 
>>>> Jonathan Morton <chromatix99@gmail.com> writes:
>>>> 
>>>>>> On 6 Mar, 2018, at 11:06 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>>>> 
>>>>>> ...on the iproute2 side the only
>>>>>> thing missing before we can attempt an upstream submission is an update
>>>>>> of the man page, as far as I can tell. Any volunteers to do that? :)
>>>>> 
>>>>> I could look into that tomorrow.
>>>> 
>>>> Awesome! Is there anything else you want to add on the kernel side
>>>> before we submit?
>>> 
>>> I think there was mention of the repo that Dave started on the last
>>> round of getting cake in upstream iproute2. There were a few man page
>>> tweaks & output formatting tweaks which ideally shouldn’t be lost.
>>> I’ve forked that fork on github here
>>> https://github.com/ldir-EDB0/iproute2-cake-next/tree/cake rebased onto
>>> release tag v4.15.0 and pulled in the the recent changes (overhead
>>> handling paradigm, xstats & the JSON output).
>>> 
>>> In theory I guess there should be no/little difference to tc-adv/master now but….
>>> 
>>> Anyway, it’s there if it’s of any interest… it’ll be the basis of what
>>> gets sent into LEDE a bit later today.
>> 
>> Please don't put something different into LEDE than what we're working
>> on upstreaming. It is difficult enough to keep track of the different
>> versions as they are. The tc-adv repo is already rebased on the upstream
>> iproute2-next, so if there is anything else that needs to be changed, it
>> should be changed in that repo and not in a fork...
> 
> I pulled in the man page changes from Dave's iproute2-next fork to the
> tc-adv repo now...
> 
> -Toke


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> There were some useful stats column re-alignment changes as well,
> wonder if you got those?

Probably not, as that code is not longer directly diff'able,
unfortunately... The json-related changes were fairly intrusive...

Eyeballing the diff, however, I *think* that the column-alignment is the
same...

Ah, great, but some of the calculations also changed :/

Could someone go through the patch below and check which is correct
(the + lines or the - lines), please? + is tc-adv, - is
iproute2-cake-next...

-Toke


@@ -137,8 +139,8 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                        interval = 1000000;
                        target   =   50000;
                } else if (strcmp(*argv, "interplanetary") == 0) {
-                       interval = 3600000000U;
-                       target   =       5000;
+                       interval = 1000000000;
+                       target   =   50000000;
 
                } else if (strcmp(*argv, "besteffort") == 0) {
                        diffserv = 1;
@@ -241,22 +243,12 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                /* Typical VDSL2 framing schemes, both over PTM */
                /* PTM has 64b/65b coding which absorbs some bandwidth */
                } else if (strcmp(*argv, "pppoe-ptm") == 0) {
-                       /* 2B PPP + 6B PPPoE + 6B dest MAC + 6B src MAC
-                        * + 2B ethertype + 4B Frame Check Sequence
-                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
-                        * + 2B TC-CRC (PTM-FCS) = 30B
-                        */
                        atm = 2;
-                       overhead += 30;
+                       overhead += 27;
                        overhead_set = true;
                } else if (strcmp(*argv, "bridged-ptm") == 0) {
-                       /* 6B dest MAC + 6B src MAC + 2B ethertype
-                        * + 4B Frame Check Sequence
-                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
-                        * + 2B TC-CRC (PTM-FCS) = 22B
-                        */
                        atm = 2;
-                       overhead += 22;
+                       overhead += 19;
                        overhead_set = true;
 
                } else if (strcmp(*argv, "via-ethernet") == 0) {
@@ -269,26 +261,8 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                         * that automatically, and is thus ignored.
                         *
                         * It would be deleted entirely, but it appears in the
-                        * stats output when the automatic compensation is
-                        * active.
-                        */
-
-               } else if (strcmp(*argv, "total_overhead") == 0) {
-                       /*
-                        * This is the overhead cake accounts for; added here so
-                        * that cake's "tc -s qdisc" output can be directly
-                        * pasted into the tc command to instantate a new cake..
+                        * stats output when the automatic compensation is active.
                         */
-                       NEXT_ARG();
-
-               } else if (strcmp(*argv, "hard_header_len") == 0) {
-                       /*
-                        * This is the overhead the kernel automatically
-                        * accounted for; added here so that cake's "tc -s
-                        * qdisc" output can be directly pasted into the tc
-                        * command to instantiate a new cake..
-                        */
-                       NEXT_ARG();
 
                } else if (strcmp(*argv, "ethernet") == 0) {
                        /* ethernet pre-amble & interframe gap & FCS
@@ -376,7 +350,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                addattr_l(n, 1024, TCA_CAKE_OVERHEAD, &overhead, sizeof(overhead));
        if (overhead_override) {
                unsigned zero = 0;
-               addattr_l(n, 1024, TCA_CAKE_ETHERNET, &zero, sizeof(zero));
+               addattr_l(n, 1024, TCA_CAKE_RAW, &zero, sizeof(zero));
        }
        if (mpu > 0)
                addattr_l(n, 1024, TCA_CAKE_MPU, &mpu, sizeof(mpu));
@@ -532,9 +508,8 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
            RTA_PAYLOAD(tb[TCA_CAKE_ACK_FILTER]) >= sizeof(__u32)) {
                ack_filter = rta_getattr_u32(tb[TCA_CAKE_ACK_FILTER]);
        }
-       if (tb[TCA_CAKE_ETHERNET] &&
-           RTA_PAYLOAD(tb[TCA_CAKE_ETHERNET]) >= sizeof(__u32)) {
-               ethernet = rta_getattr_u32(tb[TCA_CAKE_ETHERNET]);
+       if (tb[TCA_CAKE_RAW]) {
+               raw = 1;
        }
        if (tb[TCA_CAKE_RTT] &&
            RTA_PAYLOAD(tb[TCA_CAKE_RTT]) >= sizeof(__u32)) {

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 6912 bytes --]

I don’t the column alignment can be correct because the print lines don’t include a leading space, so columns can run into each other.

fprintf(f, "%12u", tst->unresponse_flows);
v
fprintf(f, " %12u", tst->unresponse_flows);

The header lines are probably wrong.  Should be as per https://github.com/ldir-EDB0/iproute2-cake-next/commit/172c116722e2902bdc69824271303dac7249f379


I no longer think the ‘total overhead’ & ‘hard head len’ keywords are needed because they’re no longer output as part of the invocation line.  At one point there was a requirement to be able to cut’n’paste the output back in as a command line input.

All of this is fixed/consolidate into one place in the repo I pointed to make sure it wasn’t lost, with the JSON changes put on top.  All the cake commits are ‘on the top’ and easy to see.  The only issue I’m seeing since the JSON is impossibly high values for some of the stats.

qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
 Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
 backlog 0b 4491256p requeues 4491272
 memory used: 78592b of 4Mb
 capacity estimate: 19900Kbit
 min/max transport layer size:    4537916 / 4537972
 min/max overhead-adjusted size:  4538000 / 4537972
 average transport hdr offset:    4538072

I’m pondering if it’s an endian issue?



> On 7 Mar 2018, at 11:28, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> There were some useful stats column re-alignment changes as well,
>> wonder if you got those?
> 
> Probably not, as that code is not longer directly diff'able,
> unfortunately... The json-related changes were fairly intrusive...
> 
> Eyeballing the diff, however, I *think* that the column-alignment is the
> same...
> 
> Ah, great, but some of the calculations also changed :/
> 
> Could someone go through the patch below and check which is correct
> (the + lines or the - lines), please? + is tc-adv, - is
> iproute2-cake-next...
> 
> -Toke
> 
> 
> @@ -137,8 +139,8 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                        interval = 1000000;
>                        target   =   50000;
>                } else if (strcmp(*argv, "interplanetary") == 0) {
> -                       interval = 3600000000U;
> -                       target   =       5000;
> +                       interval = 1000000000;
> +                       target   =   50000000;
> 
>                } else if (strcmp(*argv, "besteffort") == 0) {
>                        diffserv = 1;
> @@ -241,22 +243,12 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                /* Typical VDSL2 framing schemes, both over PTM */
>                /* PTM has 64b/65b coding which absorbs some bandwidth */
>                } else if (strcmp(*argv, "pppoe-ptm") == 0) {

> -                       /* 2B PPP + 6B PPPoE + 6B dest MAC + 6B src MAC
> -                        * + 2B ethertype + 4B Frame Check Sequence
> -                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
> -                        * + 2B TC-CRC (PTM-FCS) = 30B
> -                        */
>                        atm = 2;
> -                       overhead += 30;
> +                       overhead += 27;
the - is correct
>                        overhead_set = true;
>                } else if (strcmp(*argv, "bridged-ptm") == 0) {
> -                       /* 6B dest MAC + 6B src MAC + 2B ethertype
> -                        * + 4B Frame Check Sequence
> -                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
> -                        * + 2B TC-CRC (PTM-FCS) = 22B
> -                        */
>                        atm = 2;
> -                       overhead += 22;
> +                       overhead += 19;
>                        overhead_set = true;
> 
the - is correct
>                } else if (strcmp(*argv, "via-ethernet") == 0) {
> @@ -269,26 +261,8 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                         * that automatically, and is thus ignored.
>                         *
>                         * It would be deleted entirely, but it appears in the
> -                        * stats output when the automatic compensation is
> -                        * active.
> -                        */
> -
> -               } else if (strcmp(*argv, "total_overhead") == 0) {
> -                       /*
> -                        * This is the overhead cake accounts for; added here so
> -                        * that cake's "tc -s qdisc" output can be directly
> -                        * pasted into the tc command to instantate a new cake..
> +                        * stats output when the automatic compensation is active.
>                         */
> -                       NEXT_ARG();
> -
> -               } else if (strcmp(*argv, "hard_header_len") == 0) {
> -                       /*
> -                        * This is the overhead the kernel automatically
> -                        * accounted for; added here so that cake's "tc -s
> -                        * qdisc" output can be directly pasted into the tc
> -                        * command to instantiate a new cake..
> -                        */
> -                       NEXT_ARG();
> 
>                } else if (strcmp(*argv, "ethernet") == 0) {
>                        /* ethernet pre-amble & interframe gap & FCS
> @@ -376,7 +350,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                addattr_l(n, 1024, TCA_CAKE_OVERHEAD, &overhead, sizeof(overhead));
>        if (overhead_override) {
>                unsigned zero = 0;
> -               addattr_l(n, 1024, TCA_CAKE_ETHERNET, &zero, sizeof(zero));
> +               addattr_l(n, 1024, TCA_CAKE_RAW, &zero, sizeof(zero));
>        }
>        if (mpu > 0)
>                addattr_l(n, 1024, TCA_CAKE_MPU, &mpu, sizeof(mpu));
> @@ -532,9 +508,8 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>            RTA_PAYLOAD(tb[TCA_CAKE_ACK_FILTER]) >= sizeof(__u32)) {
>                ack_filter = rta_getattr_u32(tb[TCA_CAKE_ACK_FILTER]);
>        }
> -       if (tb[TCA_CAKE_ETHERNET] &&
> -           RTA_PAYLOAD(tb[TCA_CAKE_ETHERNET]) >= sizeof(__u32)) {
> -               ethernet = rta_getattr_u32(tb[TCA_CAKE_ETHERNET]);
> +       if (tb[TCA_CAKE_RAW]) {
> +               raw = 1;
>        }
>        if (tb[TCA_CAKE_RTT] &&
>            RTA_PAYLOAD(tb[TCA_CAKE_RTT]) >= sizeof(__u32)) {


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> I don’t the column alignment can be correct because the print lines don’t include a leading space, so columns can run into each other.
>
> fprintf(f, "%12u", tst->unresponse_flows);
> v
> fprintf(f, " %12u", tst->unresponse_flows);
>
> The header lines are probably wrong.  Should be as per
> https://github.com/ldir-EDB0/iproute2-cake-next/commit/172c116722e2902bdc69824271303dac7249f379

Ah, right; ported that change over to the tc-adv repo as well. Which
brings us down to the following diff:

diff --git a/tc/q_cake.c b/tc/q_cake.c
index e21552e8..95301b41 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -243,12 +243,22 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                /* Typical VDSL2 framing schemes, both over PTM */
                /* PTM has 64b/65b coding which absorbs some bandwidth */
                } else if (strcmp(*argv, "pppoe-ptm") == 0) {
+                       /* 2B PPP + 6B PPPoE + 6B dest MAC + 6B src MAC
+                        * + 2B ethertype + 4B Frame Check Sequence
+                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
+                        * + 2B TC-CRC (PTM-FCS) = 30B
+                        */
                        atm = 2;
-                       overhead += 27;
+                       overhead += 30;
                        overhead_set = true;
                } else if (strcmp(*argv, "bridged-ptm") == 0) {
+                       /* 6B dest MAC + 6B src MAC + 2B ethertype
+                        * + 4B Frame Check Sequence
+                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
+                        * + 2B TC-CRC (PTM-FCS) = 22B
+                        */
                        atm = 2;
-                       overhead += 19;
+                       overhead += 22;
                        overhead_set = true;
 



I assume 30 and 22 are the correct values? Could someone confirm this? :)

> qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
>  Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
>  backlog 0b 4491256p requeues 4491272
>  memory used: 78592b of 4Mb
>  capacity estimate: 19900Kbit
>  min/max transport layer size:    4537916 / 4537972
>  min/max overhead-adjusted size:  4538000 / 4537972
>  average transport hdr offset:    4538072
>
> I’m pondering if it’s an endian issue?

Are you using the latest kernel code? Those values were added in an
incompatible manner, so if you don't have userspace and kernel code in
sync they'll be garbage, basically...

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Sebastian Moeller]

Hi Toke,



> On Mar 7, 2018, at 13:59, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> I don’t the column alignment can be correct because the print lines don’t include a leading space, so columns can run into each other.
>> 
>> fprintf(f, "%12u", tst->unresponse_flows);
>> v
>> fprintf(f, " %12u", tst->unresponse_flows);
>> 
>> The header lines are probably wrong.  Should be as per
>> https://github.com/ldir-EDB0/iproute2-cake-next/commit/172c116722e2902bdc69824271303dac7249f379
> 
> Ah, right; ported that change over to the tc-adv repo as well. Which
> brings us down to the following diff:
> 
> diff --git a/tc/q_cake.c b/tc/q_cake.c
> index e21552e8..95301b41 100644
> --- a/tc/q_cake.c
> +++ b/tc/q_cake.c
> @@ -243,12 +243,22 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                /* Typical VDSL2 framing schemes, both over PTM */
>                /* PTM has 64b/65b coding which absorbs some bandwidth */
>                } else if (strcmp(*argv, "pppoe-ptm") == 0) {
> +                       /* 2B PPP + 6B PPPoE + 6B dest MAC + 6B src MAC
> +                        * + 2B ethertype + 4B Frame Check Sequence
> +                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
> +                        * + 2B TC-CRC (PTM-FCS) = 30B
> +                        */
>                        atm = 2;
> -                       overhead += 27;
> +                       overhead += 30;
>                        overhead_set = true;
>                } else if (strcmp(*argv, "bridged-ptm") == 0) {
> +                       /* 6B dest MAC + 6B src MAC + 2B ethertype
> +                        * + 4B Frame Check Sequence
> +                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
> +                        * + 2B TC-CRC (PTM-FCS) = 22B
> +                        */
>                        atm = 2;
> -                       overhead += 19;
> +                       overhead += 22;
>                        overhead_set = true;
> 
> 
> 
> 
> I assume 30 and 22 are the correct values? Could someone confirm this? :)

	As I made that change all I can confirm that at the current time I am convinced that 30 and 22 are the correct values. I did look into the ITU standard documents for VDSL and to the best of my knowledge these agree.

Best Regards
	Sebastian

> 
>> qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
>> Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
>> backlog 0b 4491256p requeues 4491272
>> memory used: 78592b of 4Mb
>> capacity estimate: 19900Kbit
>> min/max transport layer size:    4537916 / 4537972
>> min/max overhead-adjusted size:  4538000 / 4537972
>> average transport hdr offset:    4538072
>> 
>> I’m pondering if it’s an endian issue?
> 
> Are you using the latest kernel code? Those values were added in an
> incompatible manner, so if you don't have userspace and kernel code in
> sync they'll be garbage, basically...
> 
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Sebastian Moeller <moeller0@gmx.de> writes:

>> diff --git a/tc/q_cake.c b/tc/q_cake.c
>> index e21552e8..95301b41 100644
>> --- a/tc/q_cake.c
>> +++ b/tc/q_cake.c
>> @@ -243,12 +243,22 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>>                /* Typical VDSL2 framing schemes, both over PTM */
>>                /* PTM has 64b/65b coding which absorbs some bandwidth */
>>                } else if (strcmp(*argv, "pppoe-ptm") == 0) {
>> +                       /* 2B PPP + 6B PPPoE + 6B dest MAC + 6B src MAC
>> +                        * + 2B ethertype + 4B Frame Check Sequence
>> +                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
>> +                        * + 2B TC-CRC (PTM-FCS) = 30B
>> +                        */
>>                        atm = 2;
>> -                       overhead += 27;
>> +                       overhead += 30;
>>                        overhead_set = true;
>>                } else if (strcmp(*argv, "bridged-ptm") == 0) {
>> +                       /* 6B dest MAC + 6B src MAC + 2B ethertype
>> +                        * + 4B Frame Check Sequence
>> +                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
>> +                        * + 2B TC-CRC (PTM-FCS) = 22B
>> +                        */
>>                        atm = 2;
>> -                       overhead += 19;
>> +                       overhead += 22;
>>                        overhead_set = true;
>> 
>> 
>> 
>> 
>> I assume 30 and 22 are the correct values? Could someone confirm this? :)
>
> 	As I made that change all I can confirm that at the current time
> 	I am convinced that 30 and 22 are the correct values. I did look
> 	into the ITU standard documents for VDSL and to the best of my
> 	knowledge these agree.

Awesome. Pushed that change to the tc-adv repo. Which means that the
cake-specific bits of both repos are now identical; but the tc-adv repo
is up-to-date with upstream iproute2-next.

I'll see if I can cook up a patch for openwrt...

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Dave Taht]

So it sounds like all the individual pieces are sync'd again?

4.16-rc4 is out, so there are several weeks left to make the net-next
window. I note that I don't feel strongly (never had) that I should be
the one to make the net-next submission, just so long as it now gets
through checkpatch (?)... so since the code is freshest in several
other minds than mine now, can someone volunteer to submit it to
net-next?

Otherwise I'll try to clear some time to do it next week.

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Dave Taht <dave.taht@gmail.com> writes:

> So it sounds like all the individual pieces are sync'd again?
>
> 4.16-rc4 is out, so there are several weeks left to make the net-next
> window. I note that I don't feel strongly (never had) that I should be
> the one to make the net-next submission, just so long as it now gets
> through checkpatch (?)... so since the code is freshest in several
> other minds than mine now, can someone volunteer to submit it to
> net-next?
>
> Otherwise I'll try to clear some time to do it next week.

Well, I was expecting you to still be at sea ;)

So was going to submit the same(ish) patch to both netdev and openwrt;
just want to make sure there is nothing more that needs to go in...

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Dave Taht]

On Wed, Mar 7, 2018 at 7:52 AM, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Dave Taht <dave.taht@gmail.com> writes:
>
>> So it sounds like all the individual pieces are sync'd again?
>>
>> 4.16-rc4 is out, so there are several weeks left to make the net-next
>> window. I note that I don't feel strongly (never had) that I should be
>> the one to make the net-next submission, just so long as it now gets
>> through checkpatch (?)... so since the code is freshest in several
>> other minds than mine now, can someone volunteer to submit it to
>> net-next?
>>
>> Otherwise I'll try to clear some time to do it next week.
>
> Well, I was expecting you to still be at sea ;)

For those that haven't been tracking https://www.facebook.com/dtaht -

I bought an old 35' sailboat in the santa cruz, ca, harbor in December
for 1 BTC. I figured living aboard would be cheaper than california
rents, by far, and I'd be able to still work online a mile from shore,
after adding solar panels, etc.

So far neither is true, and I've sunk more energy, time and money into
making her shipshape than I'd ever imagined. Yes, it's a nice break
from fixing the internet. Yes, yes, she's called the Bufferboat, at
least tenatively - although it's federally registered as the Defiance.
She sleeps six. If anyone wants a ride or be crew in the next year...
let me know separately.

I'm increasingly unsure if the live-aboard-and-code idea is going to
work out. I might need a bigger boat, and crew. ;)

> So was going to submit the same(ish) patch to both netdev and openwrt;
> just want to make sure there is nothing more that needs to go in...

thx! All the space I had in my head for cake got eaten by the 1972
westerbeke manual.

>
> -Toke



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Pete Heist]

> On Mar 7, 2018, at 12:26 PM, Dave Taht <dave.taht@gmail.com> wrote:
> 
> For those that haven't been tracking https://www.facebook.com/dtaht -
> 
> I bought an old 35' sailboat in the santa cruz, ca, harbor in December
> for 1 BTC. I figured living aboard would be cheaper than california
> rents, by far, and I'd be able to still work online a mile from shore,
> after adding solar panels, etc.
> 
> So far neither is true, and I've sunk more energy, time and money into
> making her shipshape than I'd ever imagined. Yes, it's a nice break
> from fixing the internet. Yes, yes, she's called the Bufferboat, at
> least tenatively - although it's federally registered as the Defiance.
> She sleeps six. If anyone wants a ride or be crew in the next year...
> let me know separately.
> 
> I'm increasingly unsure if the live-aboard-and-code idea is going to
> work out. I might need a bigger boat, and crew. ;)

Awesome! I just figured that was for a trip, not a home office. :) I don’t facebook, so thanks for the update. Fortune favors the crazy (I saw on an ad in a skyway in Heathrow, which makes it true).

I’m stateside (east cost) as well for a week or so, so won’t be bloat-ing. I got here just in time for the latest nor’easter, making it my second extended power outage and second thundersnow this season (0.07% of snowstorms have thundersnow, so yeah.) Spent last night burning tomato soup over an open indoor fire made with wood stolen from a closed suburban grocery store. Will go pay for that.

More importantly, will there be another effort to upstream Cake? If so, I could offer some help when I return if it’d be useful. I was in the middle of re-configuring my lab and servers when I left.

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]



> On 7 Mar 2018, at 12:59, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> 
>> qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
>> Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
>> backlog 0b 4491256p requeues 4491272
>> memory used: 78592b of 4Mb
>> capacity estimate: 19900Kbit
>> min/max transport layer size:    4537916 / 4537972
>> min/max overhead-adjusted size:  4538000 / 4537972
>> average transport hdr offset:    4538072
>> 
>> I’m pondering if it’s an endian issue?
> 
> Are you using the latest kernel code? Those values were added in an
> incompatible manner, so if you don't have userspace and kernel code in
> sync they'll be garbage, basically…

I’m using latest commit on kernel space sch_cake cobalt branch and identical user space tc/q_cake.c to tc-adv repo.  It’s not just those new values, overhead, dropped, overlimits, requeues are all long standing parameters.

All the values, including the new, worked just before the big json conversion.



[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]



> On 7 Mar 2018, at 18:27, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> 
> 
>> On 7 Mar 2018, at 12:59, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> 
>>> qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
>>> Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
>>> backlog 0b 4491256p requeues 4491272
>>> memory used: 78592b of 4Mb
>>> capacity estimate: 19900Kbit
>>> min/max transport layer size:    4537916 / 4537972
>>> min/max overhead-adjusted size:  4538000 / 4537972
>>> average transport hdr offset:    4538072
>>> 
>>> I’m pondering if it’s an endian issue?
>> 
>> Are you using the latest kernel code? Those values were added in an
>> incompatible manner, so if you don't have userspace and kernel code in
>> sync they'll be garbage, basically…
> 
> I’m using latest commit on kernel space sch_cake cobalt branch and identical user space tc/q_cake.c to tc-adv repo.  It’s not just those new values, overhead, dropped, overlimits, requeues are all long standing parameters.
> 
> All the values, including the new, worked just before the big json conversion.

Aha, more clue - the json output ‘tc -j’ looks better (possibly correct though I’m dubious of the rate value)

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 7 Mar, 2018, at 8:27 pm, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
>> Are you using the latest kernel code? Those values were added in an
>> incompatible manner, so if you don't have userspace and kernel code in
>> sync they'll be garbage, basically…
> 
> I’m using latest commit on kernel space sch_cake cobalt branch and identical user space tc/q_cake.c to tc-adv repo.  It’s not just those new values, overhead, dropped, overlimits, requeues are all long standing parameters.
> 
> All the values, including the new, worked just before the big json conversion.

Supporting this, all five of the new fields are u16 or smaller, so are incapable of representing (or being misinterpreted as) values in the millions.

I haven't looked at the converted code yet.

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 7 Mar 2018, at 12:59, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> 
>>> qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
>>> Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
>>> backlog 0b 4491256p requeues 4491272
>>> memory used: 78592b of 4Mb
>>> capacity estimate: 19900Kbit
>>> min/max transport layer size:    4537916 / 4537972
>>> min/max overhead-adjusted size:  4538000 / 4537972
>>> average transport hdr offset:    4538072
>>> 
>>> I’m pondering if it’s an endian issue?
>> 
>> Are you using the latest kernel code? Those values were added in an
>> incompatible manner, so if you don't have userspace and kernel code in
>> sync they'll be garbage, basically…
>
> I’m using latest commit on kernel space sch_cake cobalt branch and
> identical user space tc/q_cake.c to tc-adv repo.  It’s not just those
> new values, overhead, dropped, overlimits, requeues are all long
> standing parameters.

Ah, totally missed that those were wrong as well. The values for
dropped, overlimits and requeues are not printed by the cake-specific
code at all, so there is something else dubious going on.

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Jonathan Morton]

> On 7 Mar, 2018, at 11:34 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Ah, totally missed that those were wrong as well. The values for
> dropped, overlimits and requeues are not printed by the cake-specific
> code at all, so there is something else dubious going on.

Could they be pointer values?

 - Jonathan Morton

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]



> On 8 Mar 2018, at 00:49, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 7 Mar, 2018, at 11:34 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Ah, totally missed that those were wrong as well. The values for
>> dropped, overlimits and requeues are not printed by the cake-specific
>> code at all, so there is something else dubious going on.
> 
> Could they be pointer values?
> 
> - Jonathan Morton

Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.

So, at this stage I’m going to say it’s an anomaly in my build system until someone can reproduce it.  I’ve a couple of local commits in my tree that I’ve got my eye on….

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]



> On 8 Mar 2018, at 07:59, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.
> 
> So, at this stage I’m going to say it’s an anomaly in my build system until someone can reproduce it.  I’ve a couple of local commits in my tree that I’ve got my eye on….

It’s not my local patches.  No idea.  Clever person required!


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 8 Mar 2018, at 07:59, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>> 
>> Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.
>> 
>> So, at this stage I’m going to say it’s an anomaly in my build system until someone can reproduce it.  I’ve a couple of local commits in my tree that I’ve got my eye on….
>
> It’s not my local patches.  No idea.  Clever person required!

What are you running this on? I can try doing an openwrt build and see
if it has the same issue...

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> Archer c7 v2. master branch of openwrt

Ah, great; I actually have one of those sitting on my desk that I could
potentially reflash without breaking anything too important.

In the meantime; do you get the same weird output on the
dropped/overlimit/requeues fields if you install a different qdisc than
cake?

-Toke

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]



> On 8 Mar 2018, at 10:57, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> Archer c7 v2. master branch of openwrt
> 
> Ah, great; I actually have one of those sitting on my desk that I could
> potentially reflash without breaking anything too important.
> 
> In the meantime; do you get the same weird output on the
> dropped/overlimit/requeues fields if you install a different qdisc than
> cake?

From previous email this morning:

> Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.



tc -s qdisc
qdisc noqueue 0: dev lo root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc fq_codel 0: dev eth1 root refcnt 4485644 limit 4497776p flows 4535164 quantum 4536072 target 5.0ms interval 100.0ms memory_limit 4Mb ecn
 Sent 74281480509 bytes 91321865 pkts (dropped 0, overlimits 0)
  maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
  new_flows_len 0 old_flows_len 0
qdisc noqueue 0: dev br-lan root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev eth1.2 root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev br-wifi_guest root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev eth1.15 root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev wlan1 root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev wlan0 root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev wlan1-1 root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
qdisc noqueue 0: dev wlan0-1 root refcnt 4485644
 Sent 0 bytes 0 pkts (dropped 0, overlimits 0)

And that’s on an openwrt system from commit f5b4f5f8e33624f27af9fb3f86e09084181c08ed
Author: Alif M. Ahmad <alive4ever@live.com>
Date:   Sun Feb 25 03:18:41 2018 +0000

So actually this problem has been around a little while, pre recent cake changes.



[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 5218 bytes --]



> On 8 Mar 2018, at 11:09, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> 
> 
>> On 8 Mar 2018, at 10:57, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>> 
>>> Archer c7 v2. master branch of openwrt
>> 
>> Ah, great; I actually have one of those sitting on my desk that I could
>> potentially reflash without breaking anything too important.
>> 
>> In the meantime; do you get the same weird output on the
>> dropped/overlimit/requeues fields if you install a different qdisc than
>> cake?
> 
> From previous email this morning:
> 
>> Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.
> 
> 
> 
> tc -s qdisc
> qdisc noqueue 0: dev lo root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc fq_codel 0: dev eth1 root refcnt 4485644 limit 4497776p flows 4535164 quantum 4536072 target 5.0ms interval 100.0ms memory_limit 4Mb ecn
> Sent 74281480509 bytes 91321865 pkts (dropped 0, overlimits 0)
>  maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
>  new_flows_len 0 old_flows_len 0
> qdisc noqueue 0: dev br-lan root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev eth1.2 root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev br-wifi_guest root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev eth1.15 root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev wlan1 root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev wlan0 root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev wlan1-1 root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> qdisc noqueue 0: dev wlan0-1 root refcnt 4485644
> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> 
> And that’s on an openwrt system from commit f5b4f5f8e33624f27af9fb3f86e09084181c08ed
> Author: Alif M. Ahmad <alive4ever@live.com>
> Date:   Sun Feb 25 03:18:41 2018 +0000
> 
> So actually this problem has been around a little while, pre recent cake changes.

Oh and curiously the bad values go away if you ask for json output it’s much better.  Which rather points at a ‘feature’ of the ‘print_string’ behaviour.

tc -s -j qdisc
[{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "lo",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "fq_codel",
        "handle": "0:",
        "dev": "eth1",
        "root": true,
        "refcnt": 2,
        "options": {
            "limit": 10240,
            "flows": 1024,
            "quantum": 1514,
            "target": 4999,
            "interval": 99999,
            "memory_limit": 4194304,
            "ecn": true
        } Sent 74283705614 bytes 91330210 pkts (dropped 0, overlimits 0)   maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
  new_flows_len 0 old_flows_len 0
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "br-lan",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "eth1.2",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "br-wifi_guest",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "eth1.15",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "wlan1",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "wlan0",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "wlan1-1",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    },{
        "kind": "noqueue",
        "handle": "0:",
        "dev": "wlan0-1",
        "root": true,
        "refcnt": 2,
        "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
    }
]


> 
> 
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 8 Mar 2018, at 11:09, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>> 
>> 
>> 
>>> On 8 Mar 2018, at 10:57, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>>> 
>>>> Archer c7 v2. master branch of openwrt
>>> 
>>> Ah, great; I actually have one of those sitting on my desk that I could
>>> potentially reflash without breaking anything too important.
>>> 
>>> In the meantime; do you get the same weird output on the
>>> dropped/overlimit/requeues fields if you install a different qdisc than
>>> cake?
>> 
>> From previous email this morning:
>> 
>>> Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.
>> 
>> 
>> 
>> tc -s qdisc
>> qdisc noqueue 0: dev lo root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc fq_codel 0: dev eth1 root refcnt 4485644 limit 4497776p flows 4535164 quantum 4536072 target 5.0ms interval 100.0ms memory_limit 4Mb ecn
>> Sent 74281480509 bytes 91321865 pkts (dropped 0, overlimits 0)
>>  maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
>>  new_flows_len 0 old_flows_len 0
>> qdisc noqueue 0: dev br-lan root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev eth1.2 root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev br-wifi_guest root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev eth1.15 root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev wlan1 root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev wlan0 root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev wlan1-1 root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> qdisc noqueue 0: dev wlan0-1 root refcnt 4485644
>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>> 
>> And that’s on an openwrt system from commit f5b4f5f8e33624f27af9fb3f86e09084181c08ed
>> Author: Alif M. Ahmad <alive4ever@live.com>
>> Date:   Sun Feb 25 03:18:41 2018 +0000
>> 
>> So actually this problem has been around a little while, pre recent cake changes.
>
> Oh and curiously the bad values go away if you ask for json output
> it’s much better.  Which rather points at a ‘feature’ of the
> ‘print_string’ behaviour.

Right. Well, the print_* functions are behind several levels of
pre-processor indirection, so not quite obvious what's going on here.
Don't really see why they should spit out garbage values, though.


Stephen, do you have any ideas?

-Toke


>
> tc -s -j qdisc
> [{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "lo",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "fq_codel",
>         "handle": "0:",
>         "dev": "eth1",
>         "root": true,
>         "refcnt": 2,
>         "options": {
>             "limit": 10240,
>             "flows": 1024,
>             "quantum": 1514,
>             "target": 4999,
>             "interval": 99999,
>             "memory_limit": 4194304,
>             "ecn": true
>         } Sent 74283705614 bytes 91330210 pkts (dropped 0, overlimits 0)   maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
>   new_flows_len 0 old_flows_len 0
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "br-lan",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "eth1.2",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "br-wifi_guest",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "eth1.15",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "wlan1",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "wlan0",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "wlan1-1",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     },{
>         "kind": "noqueue",
>         "handle": "0:",
>         "dev": "wlan0-1",
>         "root": true,
>         "refcnt": 2,
>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>     }
> ]
>
>
>> 
>> 
>> _______________________________________________
>> Cake mailing list
>> Cake@lists.bufferbloat.net
>> https://lists.bufferbloat.net/listinfo/cake
>
>
> Cheers,
>
> Kevin D-B
>
> 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Georgios Amanakis]

I am currently testing on my router, Archlinux with kernels
4.14.24/4.15.7, sch_cake from cobalt and tc-adv but it is behaving as
it should:

==========8<==========
qdisc cake 8001: dev ens4 root refcnt 2 bandwidth 12200Kbit besteffort
dual-dsthost wash ingress rtt 100.0ms noatm overhead 18mpu 64
 Sent 950929 bytes 4293 pkt (dropped 2, overlimits 641 requeues 0)
 backlog 0b 0p requeues 0
 memory used: 44528b of 4Mb
 capacity estimate: 12200Kbit
 min/max transport layer size:         28 /    1500
 min/max overhead-adjusted size:       64 /    1518
 average transport hdr offset:         14


qdisc cake 8002: dev ens3 root refcnt 2 bandwidth 2500Kbit besteffort
dual-srchost nat wash ack-filter rtt 100.0ms noatm overhead 18mpu 64
 Sent 254423 bytes 1712 pkt (dropped 0, overlimits 2158 requeues 0)
 backlog 0b 0p requeues 0
 memory used: 18Kb of 4Mb
 capacity estimate: 2500Kbit
 min/max transport layer size:         28 /    1500
 min/max overhead-adjusted size:       64 /    1518
 average transport hdr offset:         14
==========8<==========

George


On Thu, Mar 8, 2018 at 6:21 AM, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>
>>> On 8 Mar 2018, at 11:09, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>>>
>>>
>>>
>>>> On 8 Mar 2018, at 10:57, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>>
>>>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>>>>
>>>>> Archer c7 v2. master branch of openwrt
>>>>
>>>> Ah, great; I actually have one of those sitting on my desk that I could
>>>> potentially reflash without breaking anything too important.
>>>>
>>>> In the meantime; do you get the same weird output on the
>>>> dropped/overlimit/requeues fields if you install a different qdisc than
>>>> cake?
>>>
>>> From previous email this morning:
>>>
>>>> Definitely dubious and I’m no longer convinced it’s a cake only issue.  Looked at my AP which is running an older version of openwrt, so older cake, older kernel etc etc and all qdiscs are returning odd impossibly high values in a variety of fields.
>>>
>>>
>>>
>>> tc -s qdisc
>>> qdisc noqueue 0: dev lo root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc fq_codel 0: dev eth1 root refcnt 4485644 limit 4497776p flows 4535164 quantum 4536072 target 5.0ms interval 100.0ms memory_limit 4Mb ecn
>>> Sent 74281480509 bytes 91321865 pkts (dropped 0, overlimits 0)
>>>  maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
>>>  new_flows_len 0 old_flows_len 0
>>> qdisc noqueue 0: dev br-lan root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev eth1.2 root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev br-wifi_guest root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev eth1.15 root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev wlan1 root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev wlan0 root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev wlan1-1 root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>> qdisc noqueue 0: dev wlan0-1 root refcnt 4485644
>>> Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>>
>>> And that’s on an openwrt system from commit f5b4f5f8e33624f27af9fb3f86e09084181c08ed
>>> Author: Alif M. Ahmad <alive4ever@live.com>
>>> Date:   Sun Feb 25 03:18:41 2018 +0000
>>>
>>> So actually this problem has been around a little while, pre recent cake changes.
>>
>> Oh and curiously the bad values go away if you ask for json output
>> it’s much better.  Which rather points at a ‘feature’ of the
>> ‘print_string’ behaviour.
>
> Right. Well, the print_* functions are behind several levels of
> pre-processor indirection, so not quite obvious what's going on here.
> Don't really see why they should spit out garbage values, though.
>
>
> Stephen, do you have any ideas?
>
> -Toke
>
>
>>
>> tc -s -j qdisc
>> [{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "lo",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "fq_codel",
>>         "handle": "0:",
>>         "dev": "eth1",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {
>>             "limit": 10240,
>>             "flows": 1024,
>>             "quantum": 1514,
>>             "target": 4999,
>>             "interval": 99999,
>>             "memory_limit": 4194304,
>>             "ecn": true
>>         } Sent 74283705614 bytes 91330210 pkts (dropped 0, overlimits 0)   maxpacket 1514 drop_overlimit 0 new_flow_count 7549 ecn_mark 0
>>   new_flows_len 0 old_flows_len 0
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "br-lan",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "eth1.2",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "br-wifi_guest",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "eth1.15",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "wlan1",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "wlan0",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "wlan1-1",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     },{
>>         "kind": "noqueue",
>>         "handle": "0:",
>>         "dev": "wlan0-1",
>>         "root": true,
>>         "refcnt": 2,
>>         "options": {} Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
>>     }
>> ]
>>
>>
>>>
>>>
>>> _______________________________________________
>>> Cake mailing list
>>> Cake@lists.bufferbloat.net
>>> https://lists.bufferbloat.net/listinfo/cake
>>
>>
>> Cheers,
>>
>> Kevin D-B
>>
>> 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]



> On 8 Mar 2018, at 11:21, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
>> 
>> Oh and curiously the bad values go away if you ask for json output
>> it’s much better.  Which rather points at a ‘feature’ of the
>> ‘print_string’ behaviour.
> 
> Right. Well, the print_* functions are behind several levels of
> pre-processor indirection, so not quite obvious what's going on here.
> Don't really see why they should spit out garbage values, though.
> 
> 
> Stephen, do you have any ideas?
> 
> -Toke

Right, cracked it and it’s horrible!

print_uint is expanded thus:  Note the type of value uint64_t

              void print_color_uint(enum output_type t, enum color_attr color, const char *key, const char *fmt, uint64_t value);
static inline void print_uint      (enum output_type t,                        const char *key, const char *fmt, uint64_t value)
                 { print_color_uint(                 t, COLOR_NONE,                        key,             fmt,          value); };

So far so good.

print_color_uint expands to:

             void print_color_uint(enum output_type t, enum color_attr color, const char *key, const char *fmt, uint64_t value)
 {
  if (((t & PRINT_JSON || t & PRINT_ANY) && _jw))
    { if (!key) jsonw_uint(_jw, value);
      else      jsonw_uint_field(_jw, key, value);
    }
  else if ((!_jw && (t & PRINT_FP || t & PRINT_ANY)))
    { color_fprintf( (stdout) , color, fmt, value);
    }
 };

Again, no issue and we eventually call color_fprintf

int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
{
        int ret = 0;
        va_list args;

        va_start(args, fmt);

        if (!color_is_enabled || attr == COLOR_NONE) {
                ret = vfprintf(fp, fmt, args);
                goto end;
        }


Now, color_printf is a variable argument list function and as such is dependent upon being told the correct size of argument variables in the fmt variable.  And that’s our problem, we’re passing a 64bit integer but telling the format string that it’s ‘int’…which I’m guessing can be variable sizes depending on architecture, as can the endianness.

If we instead do (in q_cake.c)

#include <inttypes.h>

print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer size: %10" PRIu64, stnc->min_trnlen);

it works.  This needs sanity checking by a clever person.

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Luis E. Garcia]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

Nice work Kevin.

On Sat, Mar 10, 2018 at 09:56 Kevin Darbyshire-Bryant <
kevin@darbyshire-bryant.me.uk> wrote:

>
>
> > On 8 Mar 2018, at 11:21, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> >
> >>
> >> Oh and curiously the bad values go away if you ask for json output
> >> it’s much better.  Which rather points at a ‘feature’ of the
> >> ‘print_string’ behaviour.
> >
> > Right. Well, the print_* functions are behind several levels of
> > pre-processor indirection, so not quite obvious what's going on here.
> > Don't really see why they should spit out garbage values, though.
> >
> >
> > Stephen, do you have any ideas?
> >
> > -Toke
>
> Right, cracked it and it’s horrible!
>
> print_uint is expanded thus:  Note the type of value uint64_t
>
>               void print_color_uint(enum output_type t, enum color_attr
> color, const char *key, const char *fmt, uint64_t value);
> static inline void print_uint      (enum output_type t,
>     const char *key, const char *fmt, uint64_t value)
>                  { print_color_uint(                 t, COLOR_NONE,
>                 key,             fmt,          value); };
>
> So far so good.
>
> print_color_uint expands to:
>
>              void print_color_uint(enum output_type t, enum color_attr
> color, const char *key, const char *fmt, uint64_t value)
>  {
>   if (((t & PRINT_JSON || t & PRINT_ANY) && _jw))
>     { if (!key) jsonw_uint(_jw, value);
>       else      jsonw_uint_field(_jw, key, value);
>     }
>   else if ((!_jw && (t & PRINT_FP || t & PRINT_ANY)))
>     { color_fprintf( (stdout) , color, fmt, value);
>     }
>  };
>
> Again, no issue and we eventually call color_fprintf
>
> int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...)
> {
>         int ret = 0;
>         va_list args;
>
>         va_start(args, fmt);
>
>         if (!color_is_enabled || attr == COLOR_NONE) {
>                 ret = vfprintf(fp, fmt, args);
>                 goto end;
>         }
>
>
> Now, color_printf is a variable argument list function and as such is
> dependent upon being told the correct size of argument variables in the fmt
> variable.  And that’s our problem, we’re passing a 64bit integer but
> telling the format string that it’s ‘int’…which I’m guessing can be
> variable sizes depending on architecture, as can the endianness.
>
> If we instead do (in q_cake.c)
>
> #include <inttypes.h>
>
> print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer
> size: %10" PRIu64, stnc->min_trnlen);
>
> it works.  This needs sanity checking by a clever person.
>
> Cheers,
>
> Kevin D-B
>
> 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake
>

[-- Attachment #2: Type: text/html, Size: 3798 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]



> On 7 Mar 2018, at 10:31, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
>> 
> 
> Please don't put something different into LEDE than what we're working
> on upstreaming. It is difficult enough to keep track of the different
> versions as they are. The tc-adv repo is already rebased on the upstream
> iproute2-next, so if there is anything else that needs to be changed, it
> should be changed in that repo and not in a fork...
> 
> -Toke

Received & understood.  Not sending to lede… probably a good idea anyway as the main stats (not tin stats) are broken for me.  I wonder if this is because q_cake has to be imported to lede as a patch applied on top of whatever iproute2 version they’re using as the base?



Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Sebastian Moeller]

Hi Kevin,




> On Mar 7, 2018, at 12:07, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> 
> 
>> On 7 Mar 2018, at 10:31, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>>> 
>> 
>> Please don't put something different into LEDE than what we're working
>> on upstreaming. It is difficult enough to keep track of the different
>> versions as they are. The tc-adv repo is already rebased on the upstream
>> iproute2-next, so if there is anything else that needs to be changed, it
>> should be changed in that repo and not in a fork...
>> 
>> -Toke
> 
> Received & understood.  Not sending to lede… probably a good idea anyway as the main stats (not tin stats) are broken for me.  I wonder if this is because q_cake has to be imported to lede as a patch applied on top of whatever iproute2 version they’re using as the base?
> 


Will you update the sch_cake ad tc patches (from tc-adv) for lede? That would be most excellent, as currently the overhead accountin of the cake in lede/openwrt is, let's say, interesting ;) (and since I try to support people in the forums I would very much like to not having to explain this especially since Jonathan really seems ti have squashed that (but to fully confirm that, I really want to get feedback from diverse group of lede cake-eaters ;)))

Best Regards
	Sebastian


> 
> 
> Cheers,
> 
> Kevin D-B
> 
> 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
> 
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 7 Mar 2018, at 10:31, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>>> 
>> 
>> Please don't put something different into LEDE than what we're working
>> on upstreaming. It is difficult enough to keep track of the different
>> versions as they are. The tc-adv repo is already rebased on the upstream
>> iproute2-next, so if there is anything else that needs to be changed, it
>> should be changed in that repo and not in a fork...
>> 
>> -Toke
>
> Received & understood.  Not sending to lede… probably a good idea
> anyway as the main stats (not tin stats) are broken for me.  I wonder
> if this is because q_cake has to be imported to lede as a patch
> applied on top of whatever iproute2 version they’re using as the base?

As far as I can tell, the iproute2 version in openwrt is currently
4.15.0; which means that once we get this sorted out, it *should* be
straight forward to create a new patch that applies cleanly. I can do
that while preparing the upstream submission; but want to make sure we
have everything included without any version divergence first...

-Toke