[Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Toke Høiland-Jørgensen]

Add three qdisc-specific drop reasons for sch_cake:

 1) SKB_DROP_REASON_CAKE_CONGESTED
    Whenever a packet is dropped by the CAKE AQM algorithm because
    congestion is detected.

 2) SKB_DROP_REASON_CAKE_FLOOD
    Whenever a packet is dropped by the flood protection part of the
    CAKE AQM algorithm (BLUE).

 3) SKB_DROP_REASON_CAKE_OVERLIMIT
    Whenever the total queue limit for a CAKE instance is exceeded and a
    packet is dropped to make room.

Also use the existing SKB_DROP_REASON_QUEUE_PURGE in cake_clear_tin().

Reasons show up as:

perf record -a -e skb:kfree_skb sleep 1; perf script

          iperf3     665 [005]   848.656964: skb:kfree_skb: skbaddr=0xffff98168a333500 rx_sk=(nil) protocol=34525 location=__dev_queue_xmit+0x10f0 reason: CAKE_OVERLIMIT
         swapper       0 [001]   909.166055: skb:kfree_skb: skbaddr=0xffff98168280cee0 rx_sk=(nil) protocol=34525 location=cake_dequeue+0x5ef reason: CAKE_CONGESTED

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/dropreason-core.h | 18 ++++++++++++++++++
 net/sched/sch_cake.c          | 43 +++++++++++++++++++++++--------------------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index c29282fabae6cdf9dd79f698b92b4b8f57156b1e..a9be76be11ad67d6cc7175f1a643314a7dcdf0b8 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -61,6 +61,9 @@
 	FN(FQ_BAND_LIMIT)		\
 	FN(FQ_HORIZON_LIMIT)		\
 	FN(FQ_FLOW_LIMIT)		\
+	FN(CAKE_CONGESTED)		\
+	FN(CAKE_FLOOD)			\
+	FN(CAKE_OVERLIMIT)		\
 	FN(CPU_BACKLOG)			\
 	FN(XDP)				\
 	FN(TC_INGRESS)			\
@@ -329,6 +332,21 @@ enum skb_drop_reason {
 	 * exceeds its limits.
 	 */
 	SKB_DROP_REASON_FQ_FLOW_LIMIT,
+	/**
+	 * @SKB_DROP_REASON_CAKE_CONGESTED: dropped by the CAKE qdisc AQM
+	 * algorithm due to congestion.
+	 */
+	SKB_DROP_REASON_CAKE_CONGESTED,
+	/**
+	 * @SKB_DROP_REASON_CAKE_FLOOD: dropped by the flood protection part of
+	 * CAKE qdisc AQM algorithm (BLUE).
+	 */
+	SKB_DROP_REASON_CAKE_FLOOD,
+	/**
+	 * @SKB_DROP_REASON_CAKE_OVERLIMIT: dropped by CAKE qdisc when a qdisc
+	 * instance exceeds its total buffer size limit.
+	 */
+	SKB_DROP_REASON_CAKE_OVERLIMIT,
 	/**
 	 * @SKB_DROP_REASON_CPU_BACKLOG: failed to enqueue the skb to the per CPU
 	 * backlog queue. This can be caused by backlog queue full (see
diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 8d8b2db4653c0c9f271f9c1953e8c61175d8f76b..a57bdc771dd14e81fb0cdf54ca7890ac96f1e311 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -484,13 +484,14 @@ static bool cobalt_queue_empty(struct cobalt_vars *vars,
 /* Call this with a freshly dequeued packet for possible congestion marking.
  * Returns true as an instruction to drop the packet, false for delivery.
  */
-static bool cobalt_should_drop(struct cobalt_vars *vars,
-			       struct cobalt_params *p,
-			       ktime_t now,
-			       struct sk_buff *skb,
-			       u32 bulk_flows)
+static enum skb_drop_reason cobalt_should_drop(struct cobalt_vars *vars,
+					       struct cobalt_params *p,
+					       ktime_t now,
+					       struct sk_buff *skb,
+					       u32 bulk_flows)
 {
-	bool next_due, over_target, drop = false;
+	enum skb_drop_reason reason = SKB_NOT_DROPPED_YET;
+	bool next_due, over_target;
 	ktime_t schedule;
 	u64 sojourn;
 
@@ -533,7 +534,8 @@ static bool cobalt_should_drop(struct cobalt_vars *vars,
 
 	if (next_due && vars->dropping) {
 		/* Use ECN mark if possible, otherwise drop */
-		drop = !(vars->ecn_marked = INET_ECN_set_ce(skb));
+		if (!(vars->ecn_marked = INET_ECN_set_ce(skb)))
+			reason = SKB_DROP_REASON_CAKE_CONGESTED;
 
 		vars->count++;
 		if (!vars->count)
@@ -556,16 +558,17 @@ static bool cobalt_should_drop(struct cobalt_vars *vars,
 	}
 
 	/* Simple BLUE implementation.  Lack of ECN is deliberate. */
-	if (vars->p_drop)
-		drop |= (get_random_u32() < vars->p_drop);
+	if (vars->p_drop && reason == SKB_NOT_DROPPED_YET &&
+	    get_random_u32() < vars->p_drop)
+		reason = SKB_DROP_REASON_CAKE_FLOOD;
 
 	/* Overload the drop_next field as an activity timeout */
 	if (!vars->count)
 		vars->drop_next = ktime_add_ns(now, p->interval);
-	else if (ktime_to_ns(schedule) > 0 && !drop)
+	else if (ktime_to_ns(schedule) > 0 && reason == SKB_NOT_DROPPED_YET)
 		vars->drop_next = now;
 
-	return drop;
+	return reason;
 }
 
 static bool cake_update_flowkeys(struct flow_keys *keys,
@@ -1528,12 +1531,11 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
 
 	flow->dropped++;
 	b->tin_dropped++;
-	sch->qstats.drops++;
 
 	if (q->rate_flags & CAKE_FLAG_INGRESS)
 		cake_advance_shaper(q, b, skb, now, true);
 
-	__qdisc_drop(skb, to_free);
+	qdisc_drop_reason(skb, sch, to_free, SKB_DROP_REASON_CAKE_OVERLIMIT);
 	sch->q.qlen--;
 	qdisc_tree_reduce_backlog(sch, 1, len);
 
@@ -1926,7 +1928,7 @@ static void cake_clear_tin(struct Qdisc *sch, u16 tin)
 	q->cur_tin = tin;
 	for (q->cur_flow = 0; q->cur_flow < CAKE_QUEUES; q->cur_flow++)
 		while (!!(skb = cake_dequeue_one(sch)))
-			kfree_skb(skb);
+			kfree_skb_reason(skb, SKB_DROP_REASON_QUEUE_PURGE);
 }
 
 static struct sk_buff *cake_dequeue(struct Qdisc *sch)
@@ -1934,6 +1936,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 	struct cake_sched_data *q = qdisc_priv(sch);
 	struct cake_tin_data *b = &q->tins[q->cur_tin];
 	struct cake_host *srchost, *dsthost;
+	enum skb_drop_reason reason;
 	ktime_t now = ktime_get();
 	struct cake_flow *flow;
 	struct list_head *head;
@@ -2143,12 +2146,12 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 			goto begin;
 		}
 
+		reason = cobalt_should_drop(&flow->cvars, &b->cparams, now, skb,
+					    (b->bulk_flow_count *
+					     !!(q->rate_flags &
+						CAKE_FLAG_INGRESS)));
 		/* Last packet in queue may be marked, shouldn't be dropped */
-		if (!cobalt_should_drop(&flow->cvars, &b->cparams, now, skb,
-					(b->bulk_flow_count *
-					 !!(q->rate_flags &
-					    CAKE_FLAG_INGRESS))) ||
-		    !flow->head)
+		if (reason == SKB_NOT_DROPPED_YET || !flow->head)
 			break;
 
 		/* drop this packet, get another one */
@@ -2162,7 +2165,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 		b->tin_dropped++;
 		qdisc_tree_reduce_backlog(sch, 1, qdisc_pkt_len(skb));
 		qdisc_qstats_drop(sch);
-		kfree_skb(skb);
+		kfree_skb_reason(skb, reason);
 		if (q->rate_flags & CAKE_FLAG_INGRESS)
 			goto retry;
 	}

---
base-commit: 7ea2745766d776866cfbc981b21ed3cfdf50124e
change-id: 20241205-cake-drop-reason-b1661e1e7f0a

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Eric Dumazet]

On Mon, Dec 9, 2024 at 1:02 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Add three qdisc-specific drop reasons for sch_cake:
>
>  1) SKB_DROP_REASON_CAKE_CONGESTED
>     Whenever a packet is dropped by the CAKE AQM algorithm because
>     congestion is detected.
>
>  2) SKB_DROP_REASON_CAKE_FLOOD
>     Whenever a packet is dropped by the flood protection part of the
>     CAKE AQM algorithm (BLUE).
>
>  3) SKB_DROP_REASON_CAKE_OVERLIMIT
>     Whenever the total queue limit for a CAKE instance is exceeded and a
>     packet is dropped to make room.
>
> Also use the existing SKB_DROP_REASON_QUEUE_PURGE in cake_clear_tin().
>
> Reasons show up as:
>
> perf record -a -e skb:kfree_skb sleep 1; perf script
>
>           iperf3     665 [005]   848.656964: skb:kfree_skb: skbaddr=0xffff98168a333500 rx_sk=(nil) protocol=34525 location=__dev_queue_xmit+0x10f0 reason: CAKE_OVERLIMIT
>          swapper       0 [001]   909.166055: skb:kfree_skb: skbaddr=0xffff98168280cee0 rx_sk=(nil) protocol=34525 location=cake_dequeue+0x5ef reason: CAKE_CONGESTED
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Jamal Hadi Salim]

On Mon, Dec 9, 2024 at 7:02 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Add three qdisc-specific drop reasons for sch_cake:
>
>  1) SKB_DROP_REASON_CAKE_CONGESTED
>     Whenever a packet is dropped by the CAKE AQM algorithm because
>     congestion is detected.
>
>  2) SKB_DROP_REASON_CAKE_FLOOD
>     Whenever a packet is dropped by the flood protection part of the
>     CAKE AQM algorithm (BLUE).
>
>  3) SKB_DROP_REASON_CAKE_OVERLIMIT
>     Whenever the total queue limit for a CAKE instance is exceeded and a
>     packet is dropped to make room.
>
> Also use the existing SKB_DROP_REASON_QUEUE_PURGE in cake_clear_tin().
>
> Reasons show up as:
>
> perf record -a -e skb:kfree_skb sleep 1; perf script
>
>           iperf3     665 [005]   848.656964: skb:kfree_skb: skbaddr=0xffff98168a333500 rx_sk=(nil) protocol=34525 location=__dev_queue_xmit+0x10f0 reason: CAKE_OVERLIMIT
>          swapper       0 [001]   909.166055: skb:kfree_skb: skbaddr=0xffff98168280cee0 rx_sk=(nil) protocol=34525 location=cake_dequeue+0x5ef reason: CAKE_CONGESTED
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Dave Taht]

On Mon, Dec 9, 2024 at 4:02 AM Toke Høiland-Jørgensen via Cake
<cake@lists.bufferbloat.net> wrote:
>
> Add three qdisc-specific drop reasons for sch_cake:
>
>  1) SKB_DROP_REASON_CAKE_CONGESTED
>     Whenever a packet is dropped by the CAKE AQM algorithm because
>     congestion is detected.
>
>  2) SKB_DROP_REASON_CAKE_FLOOD
>     Whenever a packet is dropped by the flood protection part of the
>     CAKE AQM algorithm (BLUE).
>
>  3) SKB_DROP_REASON_CAKE_OVERLIMIT
>     Whenever the total queue limit for a CAKE instance is exceeded and a
>     packet is dropped to make room.
>
> Also use the existing SKB_DROP_REASON_QUEUE_PURGE in cake_clear_tin().
>
> Reasons show up as:
>
> perf record -a -e skb:kfree_skb sleep 1; perf script
>
>           iperf3     665 [005]   848.656964: skb:kfree_skb: skbaddr=0xffff98168a333500 rx_sk=(nil) protocol=34525 location=__dev_queue_xmit+0x10f0 reason: CAKE_OVERLIMIT
>          swapper       0 [001]   909.166055: skb:kfree_skb: skbaddr=0xffff98168280cee0 rx_sk=(nil) protocol=34525 location=cake_dequeue+0x5ef reason: CAKE_CONGESTED
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: Dave Taht <dave.taht@gmail.com>

> ---
>  include/net/dropreason-core.h | 18 ++++++++++++++++++
>  net/sched/sch_cake.c          | 43 +++++++++++++++++++++++--------------------
>  2 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index c29282fabae6cdf9dd79f698b92b4b8f57156b1e..a9be76be11ad67d6cc7175f1a643314a7dcdf0b8 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -61,6 +61,9 @@
>         FN(FQ_BAND_LIMIT)               \
>         FN(FQ_HORIZON_LIMIT)            \
>         FN(FQ_FLOW_LIMIT)               \
> +       FN(CAKE_CONGESTED)              \
> +       FN(CAKE_FLOOD)                  \
> +       FN(CAKE_OVERLIMIT)              \
>         FN(CPU_BACKLOG)                 \
>         FN(XDP)                         \
>         FN(TC_INGRESS)                  \
> @@ -329,6 +332,21 @@ enum skb_drop_reason {
>          * exceeds its limits.
>          */
>         SKB_DROP_REASON_FQ_FLOW_LIMIT,
> +       /**
> +        * @SKB_DROP_REASON_CAKE_CONGESTED: dropped by the CAKE qdisc AQM
> +        * algorithm due to congestion.
> +        */
> +       SKB_DROP_REASON_CAKE_CONGESTED,
> +       /**
> +        * @SKB_DROP_REASON_CAKE_FLOOD: dropped by the flood protection part of
> +        * CAKE qdisc AQM algorithm (BLUE).
> +        */
> +       SKB_DROP_REASON_CAKE_FLOOD,
> +       /**
> +        * @SKB_DROP_REASON_CAKE_OVERLIMIT: dropped by CAKE qdisc when a qdisc
> +        * instance exceeds its total buffer size limit.
> +        */
> +       SKB_DROP_REASON_CAKE_OVERLIMIT,
>         /**
>          * @SKB_DROP_REASON_CPU_BACKLOG: failed to enqueue the skb to the per CPU
>          * backlog queue. This can be caused by backlog queue full (see
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index 8d8b2db4653c0c9f271f9c1953e8c61175d8f76b..a57bdc771dd14e81fb0cdf54ca7890ac96f1e311 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -484,13 +484,14 @@ static bool cobalt_queue_empty(struct cobalt_vars *vars,
>  /* Call this with a freshly dequeued packet for possible congestion marking.
>   * Returns true as an instruction to drop the packet, false for delivery.
>   */
> -static bool cobalt_should_drop(struct cobalt_vars *vars,
> -                              struct cobalt_params *p,
> -                              ktime_t now,
> -                              struct sk_buff *skb,
> -                              u32 bulk_flows)
> +static enum skb_drop_reason cobalt_should_drop(struct cobalt_vars *vars,
> +                                              struct cobalt_params *p,
> +                                              ktime_t now,
> +                                              struct sk_buff *skb,
> +                                              u32 bulk_flows)
>  {
> -       bool next_due, over_target, drop = false;
> +       enum skb_drop_reason reason = SKB_NOT_DROPPED_YET;
> +       bool next_due, over_target;
>         ktime_t schedule;
>         u64 sojourn;
>
> @@ -533,7 +534,8 @@ static bool cobalt_should_drop(struct cobalt_vars *vars,
>
>         if (next_due && vars->dropping) {
>                 /* Use ECN mark if possible, otherwise drop */
> -               drop = !(vars->ecn_marked = INET_ECN_set_ce(skb));
> +               if (!(vars->ecn_marked = INET_ECN_set_ce(skb)))
> +                       reason = SKB_DROP_REASON_CAKE_CONGESTED;
>
>                 vars->count++;
>                 if (!vars->count)
> @@ -556,16 +558,17 @@ static bool cobalt_should_drop(struct cobalt_vars *vars,
>         }
>
>         /* Simple BLUE implementation.  Lack of ECN is deliberate. */
> -       if (vars->p_drop)
> -               drop |= (get_random_u32() < vars->p_drop);
> +       if (vars->p_drop && reason == SKB_NOT_DROPPED_YET &&
> +           get_random_u32() < vars->p_drop)
> +               reason = SKB_DROP_REASON_CAKE_FLOOD;
>
>         /* Overload the drop_next field as an activity timeout */
>         if (!vars->count)
>                 vars->drop_next = ktime_add_ns(now, p->interval);
> -       else if (ktime_to_ns(schedule) > 0 && !drop)
> +       else if (ktime_to_ns(schedule) > 0 && reason == SKB_NOT_DROPPED_YET)
>                 vars->drop_next = now;
>
> -       return drop;
> +       return reason;
>  }
>
>  static bool cake_update_flowkeys(struct flow_keys *keys,
> @@ -1528,12 +1531,11 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
>
>         flow->dropped++;
>         b->tin_dropped++;
> -       sch->qstats.drops++;
>
>         if (q->rate_flags & CAKE_FLAG_INGRESS)
>                 cake_advance_shaper(q, b, skb, now, true);
>
> -       __qdisc_drop(skb, to_free);
> +       qdisc_drop_reason(skb, sch, to_free, SKB_DROP_REASON_CAKE_OVERLIMIT);
>         sch->q.qlen--;
>         qdisc_tree_reduce_backlog(sch, 1, len);
>
> @@ -1926,7 +1928,7 @@ static void cake_clear_tin(struct Qdisc *sch, u16 tin)
>         q->cur_tin = tin;
>         for (q->cur_flow = 0; q->cur_flow < CAKE_QUEUES; q->cur_flow++)
>                 while (!!(skb = cake_dequeue_one(sch)))
> -                       kfree_skb(skb);
> +                       kfree_skb_reason(skb, SKB_DROP_REASON_QUEUE_PURGE);
>  }
>
>  static struct sk_buff *cake_dequeue(struct Qdisc *sch)
> @@ -1934,6 +1936,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
>         struct cake_sched_data *q = qdisc_priv(sch);
>         struct cake_tin_data *b = &q->tins[q->cur_tin];
>         struct cake_host *srchost, *dsthost;
> +       enum skb_drop_reason reason;
>         ktime_t now = ktime_get();
>         struct cake_flow *flow;
>         struct list_head *head;
> @@ -2143,12 +2146,12 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
>                         goto begin;
>                 }
>
> +               reason = cobalt_should_drop(&flow->cvars, &b->cparams, now, skb,
> +                                           (b->bulk_flow_count *
> +                                            !!(q->rate_flags &
> +                                               CAKE_FLAG_INGRESS)));
>                 /* Last packet in queue may be marked, shouldn't be dropped */
> -               if (!cobalt_should_drop(&flow->cvars, &b->cparams, now, skb,
> -                                       (b->bulk_flow_count *
> -                                        !!(q->rate_flags &
> -                                           CAKE_FLAG_INGRESS))) ||
> -                   !flow->head)
> +               if (reason == SKB_NOT_DROPPED_YET || !flow->head)
>                         break;
>
>                 /* drop this packet, get another one */
> @@ -2162,7 +2165,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
>                 b->tin_dropped++;
>                 qdisc_tree_reduce_backlog(sch, 1, qdisc_pkt_len(skb));
>                 qdisc_qstats_drop(sch);
> -               kfree_skb(skb);
> +               kfree_skb_reason(skb, reason);
>                 if (q->rate_flags & CAKE_FLAG_INGRESS)
>                         goto retry;
>         }
>
> ---
> base-commit: 7ea2745766d776866cfbc981b21ed3cfdf50124e
> change-id: 20241205-cake-drop-reason-b1661e1e7f0a
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



-- 
Dave Täht CSO, LibreQos

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Jakub Kicinski]

On Mon, 09 Dec 2024 13:02:18 +0100 Toke Høiland-Jørgensen wrote:
> Add three qdisc-specific drop reasons for sch_cake:
> 
>  1) SKB_DROP_REASON_CAKE_CONGESTED
>     Whenever a packet is dropped by the CAKE AQM algorithm because
>     congestion is detected.
> 
>  2) SKB_DROP_REASON_CAKE_FLOOD
>     Whenever a packet is dropped by the flood protection part of the
>     CAKE AQM algorithm (BLUE).
> 
>  3) SKB_DROP_REASON_CAKE_OVERLIMIT
>     Whenever the total queue limit for a CAKE instance is exceeded and a
>     packet is dropped to make room.

Eric's patch was adding fairly FQ-specific reasons, other than flood
this seems like generic AQM stuff, no? From a very quick look the
congestion looks like fairly standard AQM, overlimit is also typical
for qdics?

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Dave Taht]

On Mon, Dec 9, 2024 at 3:52 PM Jakub Kicinski via Cake
<cake@lists.bufferbloat.net> wrote:
>
> On Mon, 09 Dec 2024 13:02:18 +0100 Toke Høiland-Jørgensen wrote:
> > Add three qdisc-specific drop reasons for sch_cake:
> >
> >  1) SKB_DROP_REASON_CAKE_CONGESTED
> >     Whenever a packet is dropped by the CAKE AQM algorithm because
> >     congestion is detected.
> >
> >  2) SKB_DROP_REASON_CAKE_FLOOD
> >     Whenever a packet is dropped by the flood protection part of the
> >     CAKE AQM algorithm (BLUE).
> >
> >  3) SKB_DROP_REASON_CAKE_OVERLIMIT
> >     Whenever the total queue limit for a CAKE instance is exceeded and a
> >     packet is dropped to make room.
>
> Eric's patch was adding fairly FQ-specific reasons, other than flood
> this seems like generic AQM stuff, no? From a very quick look the
> congestion looks like fairly standard AQM, overlimit is also typical
> for qdics?

While I initially agreed with making this generic, preserving the qdisc from
where the drop came lets you safely inspect the cb block (timestamp, etc),
format of which varies by qdisc. You also get insight as to which
qdisc was dropping.

Downside is we'll end up with SKB_DROP_REASON_XXX_OVERLIMIT for
each of the qdiscs. Etc.

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



-- 
Dave Täht CSO, LibreQos

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Toke Høiland-Jørgensen]

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

> On Mon, Dec 9, 2024 at 3:52 PM Jakub Kicinski via Cake
> <cake@lists.bufferbloat.net> wrote:
>>
>> On Mon, 09 Dec 2024 13:02:18 +0100 Toke Høiland-Jørgensen wrote:
>> > Add three qdisc-specific drop reasons for sch_cake:
>> >
>> >  1) SKB_DROP_REASON_CAKE_CONGESTED
>> >     Whenever a packet is dropped by the CAKE AQM algorithm because
>> >     congestion is detected.
>> >
>> >  2) SKB_DROP_REASON_CAKE_FLOOD
>> >     Whenever a packet is dropped by the flood protection part of the
>> >     CAKE AQM algorithm (BLUE).
>> >
>> >  3) SKB_DROP_REASON_CAKE_OVERLIMIT
>> >     Whenever the total queue limit for a CAKE instance is exceeded and a
>> >     packet is dropped to make room.
>>
>> Eric's patch was adding fairly FQ-specific reasons, other than flood
>> this seems like generic AQM stuff, no? From a very quick look the
>> congestion looks like fairly standard AQM, overlimit is also typical
>> for qdics?
>
> While I initially agreed with making this generic, preserving the qdisc from
> where the drop came lets you safely inspect the cb block (timestamp, etc),
> format of which varies by qdisc. You also get insight as to which
> qdisc was dropping.
>
> Downside is we'll end up with SKB_DROP_REASON_XXX_OVERLIMIT for
> each of the qdiscs. Etc.

Yeah, I agree that a generic "dropped by AQM" reason will be too generic
without knowing which qdisc dropped it. I guess any calls directly to
kfree_skb_reason() from the qdisc will provide the calling function, but
for qdisc_drop_reason() the drop will be deferred to __dev_queue_xmit(),
so no way of knowing where the drop came from, AFAICT?

-Toke

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Jonathan Morton]

> On 10 Dec, 2024, at 10:42 am, Toke Høiland-Jørgensen via Cake <cake@lists.bufferbloat.net> wrote:
> 
>>> On Mon, 09 Dec 2024 13:02:18 +0100 Toke Høiland-Jørgensen wrote:
>>>> Add three qdisc-specific drop reasons for sch_cake:
>>>> 
>>>> 1) SKB_DROP_REASON_CAKE_CONGESTED
>>>>    Whenever a packet is dropped by the CAKE AQM algorithm because
>>>>    congestion is detected.
>>>> 
>>>> 2) SKB_DROP_REASON_CAKE_FLOOD
>>>>    Whenever a packet is dropped by the flood protection part of the
>>>>    CAKE AQM algorithm (BLUE).
>>>> 
>>>> 3) SKB_DROP_REASON_CAKE_OVERLIMIT
>>>>    Whenever the total queue limit for a CAKE instance is exceeded and a
>>>>    packet is dropped to make room.
>>> 
>>> Eric's patch was adding fairly FQ-specific reasons, other than flood
>>> this seems like generic AQM stuff, no? From a very quick look the
>>> congestion looks like fairly standard AQM, overlimit is also typical
>>> for qdics?
>> 
>> While I initially agreed with making this generic, preserving the qdisc from
>> where the drop came lets you safely inspect the cb block (timestamp, etc),
>> format of which varies by qdisc. You also get insight as to which
>> qdisc was dropping.
>> 
>> Downside is we'll end up with SKB_DROP_REASON_XXX_OVERLIMIT for
>> each of the qdiscs. Etc.
> 
> Yeah, I agree that a generic "dropped by AQM" reason will be too generic
> without knowing which qdisc dropped it. I guess any calls directly to
> kfree_skb_reason() from the qdisc will provide the calling function, but
> for qdisc_drop_reason() the drop will be deferred to __dev_queue_xmit(),
> so no way of knowing where the drop came from, AFAICT?

Would it make sense to be able to extract a "generic" code by applying a bitmask?  Leave code space for "qdisc specific" reasons within that mask.  Then people who don't care about qdisc internals can still reliably interpret the codes, even for future qdiscs.

 - Jonathan Morton

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Jakub Kicinski]

On Tue, 10 Dec 2024 09:42:55 +0100 Toke Høiland-Jørgensen wrote:
> > While I initially agreed with making this generic, preserving the qdisc from
> > where the drop came lets you safely inspect the cb block (timestamp, etc),
> > format of which varies by qdisc. You also get insight as to which
> > qdisc was dropping.
> >
> > Downside is we'll end up with SKB_DROP_REASON_XXX_OVERLIMIT for
> > each of the qdiscs. Etc.  
> 
> Yeah, I agree that a generic "dropped by AQM" reason will be too generic
> without knowing which qdisc dropped it.

Why does type of the qdisc matter if the qdisc was overlimit?

> I guess any calls directly to kfree_skb_reason() from the qdisc will
> provide the calling function, but for qdisc_drop_reason() the drop
> will be deferred to __dev_queue_xmit(), so no way of knowing where
> the drop came from, AFAICT?

Can you tell me why I'd need to inspect the skb->cb[] in cake if packet
is overlimit? Actually, none of the fields of the cb are initialized
when the packet is dropped for overlimit, AFAIU.

If someone is doing serious / advanced debug they mostly care about
access to the qdisc and can trivially check if its ops match the
expected symbol. (Speaking from experience, I've been debugging FQ
packet loss on Nov 23rd.)

If someone is just doing high level drop attribution having to list all
possible qdiscs under "qdisc discard" is purely pain.

Can we start with OVERLIMIT and CONGESTION as generic values and we can
specialize if anyone has a clear need?

Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 10 Dec 2024 09:42:55 +0100 Toke Høiland-Jørgensen wrote:
>> > While I initially agreed with making this generic, preserving the qdisc from
>> > where the drop came lets you safely inspect the cb block (timestamp, etc),
>> > format of which varies by qdisc. You also get insight as to which
>> > qdisc was dropping.
>> >
>> > Downside is we'll end up with SKB_DROP_REASON_XXX_OVERLIMIT for
>> > each of the qdiscs. Etc.  
>> 
>> Yeah, I agree that a generic "dropped by AQM" reason will be too generic
>> without knowing which qdisc dropped it.
>
> Why does type of the qdisc matter if the qdisc was overlimit?

Well, I was thinking you'd want to figure out which device it was
dropped from, but I guess you have skb->dev for that (and counters).

>> I guess any calls directly to kfree_skb_reason() from the qdisc will
>> provide the calling function, but for qdisc_drop_reason() the drop
>> will be deferred to __dev_queue_xmit(), so no way of knowing where
>> the drop came from, AFAICT?
>
> Can you tell me why I'd need to inspect the skb->cb[] in cake if packet
> is overlimit? Actually, none of the fields of the cb are initialized
> when the packet is dropped for overlimit, AFAIU.
>
> If someone is doing serious / advanced debug they mostly care about
> access to the qdisc and can trivially check if its ops match the
> expected symbol. (Speaking from experience, I've been debugging FQ
> packet loss on Nov 23rd.)
>
> If someone is just doing high level drop attribution having to list all
> possible qdiscs under "qdisc discard" is purely pain.
>
> Can we start with OVERLIMIT and CONGESTION as generic values and we can
> specialize if anyone has a clear need?

OK, I'll respin and drop CAKE from the names of those two...

-Toke