From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from g1t0026.austin.hp.com (g1t0026.austin.hp.com [15.216.28.33]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "smtp.hp.com", Issuer "VeriSign Class 3 Secure Server CA - G3" (verified OK)) by huchra.bufferbloat.net (Postfix) with ESMTPS id 7B04F20061E for ; Thu, 3 May 2012 11:17:54 -0700 (PDT) Received: from g1t0039.austin.hp.com (g1t0039.austin.hp.com [16.236.32.45]) by g1t0026.austin.hp.com (Postfix) with ESMTP id 283BAC186; Thu, 3 May 2012 18:17:53 +0000 (UTC) Received: from [16.89.64.213] (tardy.cup.hp.com [16.89.64.213]) by g1t0039.austin.hp.com (Postfix) with ESMTP id ADBFE343C7; Thu, 3 May 2012 18:18:40 +0000 (UTC) Message-ID: <4FA2CBD0.4050307@hp.com> Date: Thu, 03 May 2012 11:17:52 -0700 From: Rick Jones User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120412 Thunderbird/11.0.1 MIME-Version: 1.0 To: Dave Taht References: <1336067533-16923-1-git-send-email-dave.taht@bufferbloat.net> <1336067533-16923-2-git-send-email-dave.taht@bufferbloat.net> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Cc: codel@lists.bufferbloat.net, =?windows-1252?Q?Dave_T=E4ht?= Subject: Re: [Codel] [PATCH] Preliminary codel implementation X-BeenThere: codel@lists.bufferbloat.net X-Mailman-Version: 2.1.13 Precedence: list List-Id: CoDel AQM discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 May 2012 18:17:55 -0000 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