Re: [Codel] [PATCH] Preliminary codel implementation

[Rick Jones <rick.jones2@hp.com>]

Some nits, may not be substantive.

>> +
>> +struct tc_codel_qopt {
>> +        __u32 flags;   /* flags (e.g. ecn) */
>> +       __u32 target;   /* max delay, in us */
>> +        __u32 depth;   /* queue depth in packets */
>> +        __u32 minbytes;        /* MTU (usually) */
>> +        __u32 interval;        /* Sliding min time window width (us) */
>> +};

Perhaps include the units in target and interval - eg target_usec?  Or 
go full-CS101 with target_delay_usec and action_delay_usec (interval)?


>> +
>> +#define MS2TIME(a) (ns_to_ktime( (u64) a*1000000))
>> +#define DEFAULT_CODEL_DEPTH 1000
>> +
>> +/* Per-queue state (codel_queue_t instance variables) */
>> +
>> +struct codel_sched_data {
>> +       u32 flags;
>> +       u32 minbytes;
>> +       u32 count; /* packets dropped since we went into drop state */
>> +       bool dropping;  /* 1 if in drop state, might just add to flags */
>> +       ktime_t target;
>> +       ktime_t interval;
>> +        /* time to declare above q->target (0 if below)*/
>> +       ktime_t first_above_time;
>> +       ktime_t drop_next; /* time to drop next packet */
>> +};

Similar sort of thing here, though I suspect ktime_t implies units 
already.  Also is drop_next the time to arbitrarily drop the next packet 
or just consider it again - eg consider_drop_next?


>> +bool should_drop(struct sk_buff *skb, struct Qdisc *sch, ktime_t now)
>> +{
>> +       struct codel_sched_data *q = qdisc_priv(sch);
>> +       bool drop = 0;
>> +       if (skb == NULL) {
>> +               q->first_above_time.tv64 = 0;
>> +       } else {
>> +               ktime_t sojourn_time = ktime_sub(now, get_enqueue_time(skb));
>> +               if (sojourn_time.tv64<
>> +                   q->target.tv64 || sch->qstats.backlog<  q->minbytes) {
>> +/* went below so we’ll stay below for at least q->interval */
>> +                       q->first_above_time.tv64 = 0;
>> +               } else {
>> +                       if (q->first_above_time.tv64 == 0) {
>> +
>> +/* just went above from below. If we stay above
>> + * for at least q->interval we’ll say it’s ok to drop
>> + */

Indentation on the comment?

>> +                               q->first_above_time =
>> +                                       ktime_add(now,q->interval);
>> +                       } else if (now.tv64>= q->first_above_time.tv64) {
>> +                               drop = 1;
>> +                       }
>> +               }
>> +       }
>> +       return drop;
>> +}
>> +
>> +static struct sk_buff *codel_dequeue(struct Qdisc *sch)
>> +{
>> +       struct codel_sched_data *q = qdisc_priv(sch);
>> +       struct sk_buff *skb = codel_dequeue_head(sch);
>> +       ktime_t now;
>> +       bool drop;
>> +       if (skb == NULL) {
>> +               q->dropping = 0;
>> +               q->first_above_time.tv64 = 0;
>> +               return skb;
>> +       }
>> +       now = ktime_get();
>> +       drop = should_drop(skb, sch, now);
>> +       if (q->dropping) {
>> +               if (drop) {
>> +/* sojourn time below target - leave dropping state */
>> +                       q->dropping = 0;
>> +               } else if (now.tv64>= q->drop_next.tv64) {
>> +/*
>> + * It’s time for the next drop. Drop the current packet and dequeue the next.
>> + * The dequeue might take us out of dropping state. If not, schedule the
>> + * next drop. A large backlog might result in drop rates so high that the next
>> + * drop should happen now, hence the ‘while’ loop.
>> + */

Comment indentation?

rick