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

[Dave Taht <dave.taht@gmail.com>]

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