Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Felix Fietkau <nbd@nbd.name>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
Subject: Re: [PATCH 1/7] net/fq_impl: bulk-free packets from a flow on
 overmemory
In-Reply-To: <99ee36a1-4e30-194f-5101-0dc3d4d30a46@nbd.name>
References: <20201216204316.44498-1-nbd@nbd.name>
 <400936f158c59a25758ca05503c271710100476f.camel@sipsolutions.net>
 <99ee36a1-4e30-194f-5101-0dc3d4d30a46@nbd.name>
Date:   Thu, 17 Dec 2020 13:09:22 +0100
Message-ID: <877dpgbpn1.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-12-16 21:54, Johannes Berg wrote:
>> On Wed, 2020-12-16 at 21:43 +0100, Felix Fietkau wrote:
>>> 
>>> +static int fq_flow_drop(struct fq *fq, struct fq_flow *flow,
>>> +			fq_skb_free_t free_func)
>>> +{
>>> +	unsigned int packets = 0, bytes = 0, truesize = 0;
>>> +	struct fq_tin *tin = flow->tin;
>>> +	struct sk_buff *skb;
>>> +	int pending;
>>> +
>>> +	lockdep_assert_held(&fq->lock);
>>> +
>>> +	pending = min_t(int, 32, skb_queue_len(&flow->queue) / 2);
>>> 
>> 
>> Why 32?
> I guess I forgot got make it configurable. sch_fq_codel uses 64, but
> that seemed a bit excessive to me.

I'm not sure it's worth a configuration knob. It's basically an
arbitrary choice anyway, and only kicks in when an unresponsive flows
keeps flooding a queue to the point of overflow (if it's many smaller
flows the "never drop more than half a flow's backlog" should keep it
from being excessive).

This (hopefully) only happens relatively rarely and hitting it with a
really big hammer is the right thing to do in such a case to keep the
box from falling over. Not sure if 32 or 64 makes much difference; guess
it depends on the CPU-to-bandwidth ratio of the particular machine.

-Toke