Re: [PATCH 04/11] mt76: add utility functions for deferring work to a kernel thread

[Kalle Valo <kvalo@codeaurora.org>]

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