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;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org B78C5C433CA
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Felix Fietkau <nbd@nbd.name>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [PATCH 04/11] mt76: add utility functions for deferring work to a kernel thread
References: <20200908211756.15998-1-nbd@nbd.name>
        <20200908211756.15998-4-nbd@nbd.name>
        <010101747c3b747d-09af9fc1-2e74-400f-89f5-fae71b7d163b-000000@us-west-2.amazonses.com>
        <7e2b25d9-c6cc-1c78-a96f-e60604408578@nbd.name>
Date:   Mon, 14 Sep 2020 10:55:08 +0300
In-Reply-To: <7e2b25d9-c6cc-1c78-a96f-e60604408578@nbd.name> (Felix Fietkau's
        message of "Sat, 12 Sep 2020 07:05:59 +0200")
Message-ID: <87363kpzc3.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-09-11 10:15, Kalle Valo wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> In order to avoid keeping work like tx scheduling pinned to the CPU it was
>>> scheduled from, it makes sense to switch from tasklets to kernel threads.
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> 
>> [...]
>> 
>>> --- a/drivers/net/wireless/mediatek/mt76/util.c
>>> +++ b/drivers/net/wireless/mediatek/mt76/util.c
>>> @@ -110,4 +110,32 @@ int mt76_get_min_avg_rssi(struct mt76_dev *dev, bool ext_phy)
>>>  }
>>>  EXPORT_SYMBOL_GPL(mt76_get_min_avg_rssi);
>>>  
>>> +int __mt76_worker_fn(void *ptr)
>>> +{
>>> +	struct mt76_worker *w = ptr;
>>> +
>>> +	while (!kthread_should_stop()) {
>>> +		set_current_state(TASK_INTERRUPTIBLE);
>>> +
>>> +		if (kthread_should_park()) {
>>> +			kthread_parkme();
>>> +			continue;
>>> +		}
>>> +
>>> +		if (!test_and_clear_bit(MT76_WORKER_SCHEDULED, &w->state)) {
>>> +			schedule();
>>> +			continue;
>>> +		}
>>> +
>>> +		set_bit(MT76_WORKER_RUNNING, &w->state);
>>> +		set_current_state(TASK_RUNNING);
>>> +		w->fn(w);
>>> +		cond_resched();
>>> +		clear_bit(MT76_WORKER_RUNNING, &w->state);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(__mt76_worker_fn);
>> 
>> So how is this better than, for example,
>> create_singlethread_workqueue()? And if this is better, shouldn't it be
>> part of workqueue.h instead of every driver reinventing the wheel?
>
> Unlike a workqueue, this one only allows one fixed worker function to be
> executed by the worker thread. Because of that, there is less locking
> and less code for scheduling involved.
> In fact, the function that schedules the worker is small enough that
> it's just a simple inline function.
> The difference matters in this case, because the tx worker is scheduled
> very often in a hot path.
> I don't think it fits into workqueue.h (because of the lack of
> separation between workqueue and work struct), and I don't know if other
> drivers need this, so let's keep it in mt76 and only move to a generic
> API if we find another user for it.

Ok, fair enough. But please add this info to the commit log so the
reasoning is properly documented.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches