On 2020-09-11 10:15, Kalle Valo wrote:
> Felix Fietkau <nbd@xxxxxxxx> 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@xxxxxxxx>
>
> [...]
>
>> --- 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.
- Felix
