From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x243.google.com (mail-lj1-x243.google.com [IPv6:2a00:1450:4864:20::243]) (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 14FF63CB35 for ; Fri, 15 Nov 2019 21:22:09 -0500 (EST) Received: by mail-lj1-x243.google.com with SMTP id g3so12633014ljl.11 for ; Fri, 15 Nov 2019 18:22:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=hGU8vg18c8rykU4X+f/twEM1FcyACBPR6hxaYpz2ycI=; b=haU6P4NnkHLAULTq1MlP4/cQUVT8uZPmA02AsE89LVyEYZDnUKZnYO17kKmq5kJY3i sk+tw06YePIuTVqNwKnzeD/dAy3N9hk4P/cDAcMfWMfKhDYzb7quzmq37y83lGbKOjvp fAXxW2mynCyAP+qtdJw5ayq2EnbHMtct8WIKpfkhSzuljHzoiQ+1PDeEGmgWz+WN2uV1 7BfC7omO3BB8lTX2rqB9HeFaeBhOVObsQn8UdJLkFA2xl57mMb9L/hvNeDtUHp3p2euF ov/N3Jk6mdhaHmhiYNx8sREayhl+xNhrDe9Ar++nnSS6QA9AaAGNvaUMMd8sN+erd2n5 5e+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=hGU8vg18c8rykU4X+f/twEM1FcyACBPR6hxaYpz2ycI=; b=nGJQtGwQ1EV5K5ojgDRibwsVmg0YgxpchwZMqAqJ7nxG8bsPaZ8wzs1CR8ocd0cxgA 22zljDnezhs5SIkZnu3mMlrTqsIFsZB4ZO6HujhBrOOE0UPMIEc7KHafycQN2HLwpUxQ cevUa9xrlcv+Q+bdsBlfcWfwYnbd9CFzyLb2eT+B1O716/f0yGnAuUllVzjCrxQXTl4N p2Q1vICEh9ZlYAe4fC3WjaVhs5GN48f+9GJJ8VXu25/SAdJmIaDllkDvbfbC6vOHj8Vz qaTiKE3gqfJjbV1jmg9Wba6GkSRhm09o5NhwMSNXeTx6kOkBFQizV4Ch+WB3eEh/JY40 yuNg== X-Gm-Message-State: APjAAAUfhXjNVHrLyZxnY6DLxpI+O4nG0zd8QCwQO/lJ3+SBhz+VpZT8 svUNICvz2fdqS/LKzrtw7LDovPbi5BDIYkT9cl/FcQ== X-Google-Smtp-Source: APXvYqx8G4+RGf76UqHpeCtjAp76KchAOEGDtObnaGI+4ErQc6JS5aHwHZtSHWi0I0LQV+1FHzxm8UsJqKits+jEKzs= X-Received: by 2002:a2e:9ad8:: with SMTP id p24mr1808659ljj.114.1573870927439; Fri, 15 Nov 2019 18:22:07 -0800 (PST) MIME-Version: 1.0 References: <20191115014846.126007-1-kyan@google.com> <20191115014846.126007-3-kyan@google.com> <87k181mh7f.fsf@toke.dk> In-Reply-To: <87k181mh7f.fsf@toke.dk> From: Kan Yan Date: Fri, 15 Nov 2019 18:21:55 -0800 Message-ID: To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Cc: Johannes Berg , linux-wireless , Make-Wifi-fast , Felix Fietkau , Yibo Zhao , John Crispin , Lorenzo Bianconi , Rajkumar Manoharan , Kevin Hayes Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [Make-wifi-fast] [v8 PATCH 2/2] mac80211: Implement Airtime-based Queue Limit (AQL) 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: Sat, 16 Nov 2019 02:22:09 -0000 > > + if (sta) { > > + atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending); > > + tx_pending =3D atomic_read(&sta->airtime[ac].aql_tx_pendi= ng); > This is still racy, since you're splitting it over two calls; you'll > need to use atomic_sub_return() instead. > I figure we've converged now to the point where it actually makes sense > to collect everything back into a single series; so I can just fix this > and re-send the full series. Thanks for help fixing this. Yes, atomic_sub_return() is better. > > > > + if (WARN_ONCE(tx_pending < 0, > > + "STA %pM AC %d txq pending airtime underflo= w: %u, %u", > > + sta->addr, ac, tx_pending, tx_airtime)) > > + atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, > > + tx_pending, 0); > This could still fail if there's a concurrent modification (and you're > not checking the return of the cmpxchg). But at least it won't clobber > any updated value, so I guess that is acceptable since we're in "should > never happen" territory here :) I did this on purpose since I really don't like adding a loop to retry here. If aql_tx_pending indeed goes negative (should never happens and we got WARN_ONCE() to catch it) and the subsequent atomic_cmpxchg() failed (rare racing occasions), it is still ok. In this case, aql_tx_pending carries a negative offset and will be reset in one of the calls to ieee80211_sta_update_pending_airtime() later. aql_tx_pending being negative doesn't have much side-effects, such as causing txq stuck. On Fri, Nov 15, 2019 at 4:56 AM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > Kan Yan writes: > > > In order for the Fq_CoDel algorithm integrated in mac80211 layer to ope= rate > > effectively to control excessive queueing latency, the CoDel algorithm > > requires an accurate measure of how long packets stays in the queue, AK= A > > sojourn time. The sojourn time measured at the mac80211 layer doesn't > > include queueing latency in the lower layer (firmware/hardware) and CoD= el > > expects lower layer to have a short queue. However, most 802.11ac chips= ets > > offload tasks such TX aggregation to firmware or hardware, thus have a = deep > > lower layer queue. > > > > Without a mechanism to control the lower layer queue size, packets only > > stay in mac80211 layer transiently before being sent to firmware queue. > > As a result, the sojourn time measured by CoDel in the mac80211 layer i= s > > almost always lower than the CoDel latency target, hence CoDel does lit= tle > > to control the latency, even when the lower layer queue causes excessiv= e > > latency. > > > > The Byte Queue Limits (BQL) mechanism is commonly used to address the > > similar issue with wired network interface. However, this method cannot= be > > applied directly to the wireless network interface. "Bytes" is not a > > suitable measure of queue depth in the wireless network, as the data ra= te > > can vary dramatically from station to station in the same network, from= a > > few Mbps to over Gbps. > > > > This patch implements an Airtime-based Queue Limit (AQL) to make CoDel = work > > effectively with wireless drivers that utilized firmware/hardware > > offloading. AQL allows each txq to release just enough packets to the l= ower > > layer to form 1-2 large aggregations to keep hardware fully utilized an= d > > retains the rest of the frames in mac80211 layer to be controlled by th= e > > CoDel algorithm. > > > > Signed-off-by: Kan Yan > > [ Toke: Keep API to set pending airtime internal, fix nits in commit ms= g ] > > Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen > > --- > [...] > > > + if (sta) { > > + atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending); > > + tx_pending =3D atomic_read(&sta->airtime[ac].aql_tx_pendi= ng); > > This is still racy, since you're splitting it over two calls; you'll > need to use atomic_sub_return() instead. > > I figure we've converged now to the point where it actually makes sense > to collect everything back into a single series; so I can just fix this > and re-send the full series. > > > + if (WARN_ONCE(tx_pending < 0, > > + "STA %pM AC %d txq pending airtime underflo= w: %u, %u", > > + sta->addr, ac, tx_pending, tx_airtime)) > > + atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, > > + tx_pending, 0); > > This could still fail if there's a concurrent modification (and you're > not checking the return of the cmpxchg). But at least it won't clobber > any updated value, so I guess that is acceptable since we're in "should > never happen" territory here :) > > -Toke >