Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Kalle Valo <kvalo@kernel.org>]

Ping-Ke Shih <pkshih@realtek.com> writes:

> On Tue, 2023-04-04 at 10:37 +0800, Ping-Ke Shih wrote:
>
>> On Mon, 2023-04-03 at 13:32 +0300, Kalle Valo wrote:
>> > I would expect that there's polling if you are waiting something from
>> > hardware, or maybe when implementing a spin lock, but not when waiting
>> > for another kernel thread. This just doesn't feel right but I don't have
>> > time to propose a good alternative either, sorry.
>> > 
>> 
>> I have found a solution that uses an owner variable with a spin lock
>> to determine which side to free completion object. Simply show two use
>> cases below:
>> 
>> Use case 1: (normal case; free completion object by work 1)
>>     work 1                                work 2
>>     wait
>>                                           (spin_lock)
>>                                           complete
>>                                           check & set owner --> owner = work1
>> 		                          (spin_unlock)
>>     wait ok
>>     (spin_lock)
>>     check & check owner --> free by work 1
>>     (spin_unlock)
>>     free completion
>> 
>> 
>> Use case 2: (timeout case; free completion object by work 2)
>>     work 1                                work 2
>>     wait
>>     wait timeout
>>     (spin_lock)
>>     check & set owner --> owner = work 2
>>     (spin_unlock)
>>                                          (spin_lock)
>>                                          completion
>>                                          check & set owner --> free by work 2
>>                                          (spin_unlock)
>>                                          free completion
>> 
>> I will apply this by v5.
>> 
>
> We have a better idea that use kfree_rcu() to free completion, so no
> need spin_lock(). Then, the use cases become below, and I have sent
> this change by v6.
>
> Use case 1: (normal case)
>     work 1                                work 2
>     (rcu_assign_pointer(wait))
>     wait
>                                           (rcu_read_lock)
>                                           wait = rcu_dereference();
>                                           if (wait)    --> wait != NULL
>                                               complete
>                                           (rcu_read_unlock)
>     wait ok
>     (rcu_assign_pointer(NULL))
>     kfree_rcu(completion)
>
>
> Use case 2: (timeout case)
>     work 1                                work 2
>     (rcu_assign_pointer(wait))
>     wait
>     wait timeout
>     (rcu_assign_pointer(NULL))
>     kfree_rcu(completion)
>                                           (rcu_read_lock)
>                                           wait = rcu_dereference();
>                                           if (wait)    --> wait == NULL
>                                               complete
>                                           (rcu_read_unlock)

Thanks, I haven't had a chance to look at v6 yet (nor v5 for that
matter) but at least based on this mail your idea looks much better than
polling.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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