From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::234]) (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 85D693CC2A for ; Wed, 6 Apr 2016 01:35:48 -0400 (EDT) Received: by mail-wm0-x234.google.com with SMTP id 191so45328947wmq.0 for ; Tue, 05 Apr 2016 22:35:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tieto.com; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-transfer-encoding; bh=5j51HyC3qJFUyGQHxRy6qrl4u9YrMi+zgRKdY1sVmwA=; b=ztA+voYvCRGkDXPWu1RqxUT0NI9QF8JnqbyYC9cgrJoX6cFdWG5iV9j0NxHgsb4S78 ui4Spzvn95cJ8hxK5oJsBMXNdPXz6wvYw8BCGPbcGvV0cN3Y6EeiWxayaK8bSzweswDG 8uMCEEWeuAH62msmTLgH1O5jRCbJtOEXdNisU= 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:content-transfer-encoding; bh=5j51HyC3qJFUyGQHxRy6qrl4u9YrMi+zgRKdY1sVmwA=; b=Pr+QrlIeIjSKjLDh2C8gvdMflU70al+5Bq3PNAU42rYCcAr4U1w8Zb8A7xtzRAURA0 LszeEJWlTRz4O4jD0r+nFDxEWW7UYK1M/t/59xVH+huUfupVze+VBP9lQj1v9ezOOvbE LDM5bSZ7EmSNSa47dnONS3SJHynhoyi0bEsLhDEVn2JJJ4arqWclJiDTU92H5mKwoMxr Q3koijgXSv0dhq9VaonOKYNaMlaqVom8P0hNUb0JphTzsxF+B1PPvW1z1QwUwxwaFZYa a4PaQxbAyrUAz4ebf2kvCkuEv8itMB9fRkodvDQBdK3I1RFvV3M3Sv9WvkNwqzpHvbcI zs4Q== X-Gm-Message-State: AD7BkJLIQUXMTWIgrHyqynEbU6e9nup3RGdSBvPQ2nYgSfm/YzkvfGN4A43xAPducFru/02Aqho+BJwyv/KlfR+NNmMr1mpXQ2nyhKs+7L4BK0L3j6gPB4dpclsrJfAXD0SsqsxCYYsQrMHGOkKZPGWatyULT+Oi66MSlA== MIME-Version: 1.0 X-Received: by 10.194.157.65 with SMTP id wk1mr14317232wjb.83.1459920947330; Tue, 05 Apr 2016 22:35:47 -0700 (PDT) Received: by 10.194.115.3 with HTTP; Tue, 5 Apr 2016 22:35:47 -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: Wed, 6 Apr 2016 07:35:47 +0200 Message-ID: From: Michal Kazior To: Johannes Berg Cc: linux-wireless , Dave Taht , make-wifi-fast@lists.bufferbloat.net, codel@lists.bufferbloat.net Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-DomainID: tieto.com 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: Wed, 06 Apr 2016 05:35:48 -0000 On 5 April 2016 at 15:57, 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. I just wanted to follow the suggested/implied usage of codel code and keep modifications to a minimum. I could very well just assimilate it if you wish. >> 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? The new/old flows is follows the same principle as net/sched/sch_fq_codel.c The embedded flow is for possible collisions, explained below. Nevertheless I'll add more comments on what-is-what-and-why. >> + 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? Sure, I'll change it. >> +/** >> + * struct txq_flow - per traffic flow queue >> + * >> + * This structure is used to distinguish and queue different traffic fl= ows >> + * separately for fair queueing/AQM purposes. >> + * >> + * @txqi: txq_info structure it is associated at given time > > Do we actually have to keep that? It's on a list per txqi, no? It's used to track ownership. Packets can be destined to different stations/txqs. At enqueue time I do a partial hash of a packet to get an "index" which I then use to address a txq_flow from per-radio list (out of 4096 of them). You can end up with a situtation like this: - packet A hashing to X destined to txq P which is VI - packet B hashing to X destined to txq Q which is BK You can't use the same txq_flow for both A and B because you want to maintain packets per txqs more than you want to maintain them per flow (you don't want to queue BK traffic onto VI or vice versa as an artifact, do you? ;). When a txq_flow doesn't have a txqi it is bound. Later, if a collision happens (i.e. resulting txq_flow has non-NULL txqi) the "embedded" per-txq flow is used: struct txq_info { - struct sk_buff_head queue; + struct txq_flow flow; // <--- this When txq_flow becomes empty its txqi is reset. The embedded flow is otherwise treated like any other flow, i.e. it can be linked to old_flows and new_flows. >> + * @flowchain: can be linked to other flows for RR purposes > > RR? Round-robin. Assuming it's correct to call fq_codel an RR scheme? >> +void ieee80211_teardown_flows(struct ieee80211_local *local) >> +{ >> + struct ieee80211_fq *fq =3D &local->fq; >> + struct ieee80211_sub_if_data *sdata; >> + struct sta_info *sta; >> + int i; >> + >> + if (!local->ops->wake_tx_queue) >> + return; >> + >> + list_for_each_entry_rcu(sta, &local->sta_list, list) >> + for (i =3D 0; i < IEEE80211_NUM_TIDS; i++) >> + ieee80211_purge_txq(local, >> + to_txq_info(sta->sta.txq[i])); >> + >> + list_for_each_entry_rcu(sdata, &local->interfaces, list) >> + ieee80211_purge_txq(local, to_txq_info(sdata->vif.txq)); > > Using RCU iteration here seems rather strange, since it's a teardown > flow? That doesn't seem necessary, since it's control path and must be > holding appropriate locks anyway to make sure nothing is added to the > lists. You're probably right. I'll look into changing it. > >> + skb =3D codel_dequeue(flow, >> + &flow->backlog, >> + 0, >> + &flow->cvars, >> + &fq->cparams, >> + codel_get_time(), >> + false); > > What happened here? :) I'm not a huge fan of wrapping functions with a lot of (ugly-looking) arguments. I can make it a different ugly if you want :) >> + if (!skb) { >> + if ((head =3D=3D &txqi->new_flows) && >> + !list_empty(&txqi->old_flows)) { >> + list_move_tail(&flow->flowchain, &txqi->old_flows)= ; >> + } else { >> + list_del_init(&flow->flowchain); >> + flow->txqi =3D NULL; >> + } >> + goto begin; >> + } > > Ouch. Any way you can make that easier to follow? This follows net/sched/sch_fq_codel.h. I can put up a comment to explain what it's supposed to do? Micha=C5=82