From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-x231.google.com (mail-oi0-x231.google.com [IPv6:2607:f8b0:4003:c06::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 0E24E3CC28; Tue, 5 Apr 2016 10:32:12 -0400 (EDT) Received: by mail-oi0-x231.google.com with SMTP id p188so19536696oih.2; Tue, 05 Apr 2016 07:32:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=taO6lvcu5t5q7aG54GXB4E3FrCs/S7QieCnuxTTX1Qc=; b=RLdsqw/wkr+dhwJbbYtXY538+rXZo7ZGiX0aCqO3Ht27tTT+inVN4dzMmoaC3i6qsj fuhmq/7hPikGwFcPHx4hi8KfoJRDvMw2CyXvyMxx/UiBzHkZcrLDzm7vrOarempv2rQo 1W8jyJ6tozOld/T3ows84VyfKaBnicpYeoVNYA7HTVbK0MWU/DsbOF8cse65TZ1tLo25 ub8A7L/ssPMh3VKG0OHJ3KXIgRTro9zOgpYWxV051pEG5KZFp2esGMLJQ43WSDjFkRCM c+WPWTWPK6sy2mvL+qhniAhHsLDJUi+DzHeuHoDNm50gpTCGWpxTYXs12oe9uep7H73y Hi/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=taO6lvcu5t5q7aG54GXB4E3FrCs/S7QieCnuxTTX1Qc=; b=VT6J7um4/26s3QOXkVyXnHpKV6I63AeUAW1HJS2xCFzq+tAtVgBvP9nJeAntkg1ZWF TpGRI/pBt8pVe+/33ZQ9EdFSfq7hdhzIW3sH+L4fn3Onx+cmF/UCR6tvSHMUXQb3nxfX 5FkMNpaU6PjsQqEAXYTXCyeNHXykX5hgmGABh/LviVxVf9lPIspwONOqeZV/KGfswhZM qZJei3yZcxNMFrM1E66U+io5P55jAN8deYGcHIrY9PKiVZHOiuUH8Ae/PWuXcEqzE/RR Ms67nMFk3sJa3fbm0giiIRjhJ8yfjka82C5WUlSpn6geFBhKqT2oCXk0I0beVaw1l2l5 vISA== X-Gm-Message-State: AD7BkJIdTAmOGw2qiM7Oru8xHpo8hRd1af4mWUr/ayjy9KQVnR+TP+ZUiiLoxYyuoK5mnvW5v1CYtbPX6jjn0g== MIME-Version: 1.0 X-Received: by 10.157.57.131 with SMTP id y3mr20824331otb.169.1459866732342; Tue, 05 Apr 2016 07:32:12 -0700 (PDT) Received: by 10.202.79.215 with HTTP; Tue, 5 Apr 2016 07:32:12 -0700 (PDT) In-Reply-To: <1459864656.18188.60.camel@sipsolutions.net> References: <1458898052-20601-1-git-send-email-michal.kazior@tieto.com> <1459420104-31554-1-git-send-email-michal.kazior@tieto.com> <1459420104-31554-2-git-send-email-michal.kazior@tieto.com> <1459864656.18188.60.camel@sipsolutions.net> Date: Tue, 5 Apr 2016 07:32:12 -0700 Message-ID: From: Dave Taht To: Johannes Berg Cc: Michal Kazior , linux-wireless , make-wifi-fast@lists.bufferbloat.net, "codel@lists.bufferbloat.net" Content-Type: text/plain; charset=UTF-8 Subject: Re: [Make-wifi-fast] [PATCHv2 1/2] mac80211: implement fair queuing per txq X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Apr 2016 14:32:13 -0000 thx for the review! On Tue, Apr 5, 2016 at 6:57 AM, Johannes Berg wrote: > On Thu, 2016-03-31 at 12:28 +0200, Michal Kazior wrote: > >> +++ b/net/mac80211/codel.h >> +++ b/net/mac80211/codel_i.h > > Do we really need all this code in .h files? It seems very odd to me to > have all the algorithm implementation there rather than a C file, you > should (can?) only include codel.h into a single C file anyway. The hope had been the original codel.h would have been reusable, which is not the case at present. > >> struct txq_info { >> - struct sk_buff_head queue; >> + struct txq_flow flow; >> + struct list_head new_flows; >> + struct list_head old_flows; > > This is confusing, can you please document that? Why are there two > lists of flows, *and* an embedded flow? Is the embedded flow on any of > the lists? To explain the new and old flow concepts, there's https://tools.ietf.org/html/draft-ietf-aqm-fq-codel-06 which is in the ietf editors queue for final publication and doesn't have a final name yet. The embedded flow concept is michal's and I'm not convinced it's the right idea as yet. > >> + u32 backlog_bytes; >> + u32 backlog_packets; >> + u32 drop_codel; > > Would it make some sense to at least conceptually layer this a bit? > I.e. rather than calling this "drop_codel" call it "drop_congestion" or > something like that? Is there a more generic place overall in ieee80211 to record per-sta backlogs, drops and marks? >> + skb = codel_dequeue(flow, >> + &flow->backlog, >> + 0, >> + &flow->cvars, >> + &fq->cparams, >> + codel_get_time(), >> + false); > > What happened here? :) Magic. > >> + if (!skb) { >> + if ((head == &txqi->new_flows) && >> + !list_empty(&txqi->old_flows)) { >> + list_move_tail(&flow->flowchain, &txqi->old_flows); >> + } else { >> + list_del_init(&flow->flowchain); >> + flow->txqi = NULL; >> + } >> + goto begin; >> + } > > Ouch. Any way you can make that easier to follow? It made my brain hurt in the original code, too, but it is eric optimizing out cycles at his finest. if the the new_flows list is expired or done, switch to the old_flows list, if the old_flows list is done, go try selecting another queue to pull from (which may or may not exist). see the pending rfc for a more elongated version. > > johannes