Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "Bernie Huang" <phhuang@realtek.com>
Subject: Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs
References: <20230310034631.45299-1-pkshih@realtek.com>
        <20230310034631.45299-3-pkshih@realtek.com>
        <87v8j2mmqt.fsf@kernel.org>
        <360e6dd64e3645c68742fc4c603b3c2b@realtek.com>
        <875yadb6i1.fsf@kernel.org>
        <761e605b96d734881dc51be4679f3a04c75abb89.camel@realtek.com>
        <56831ba4f216daee09b3d9c9a7deaf5810cade34.camel@realtek.com>
Date:   Wed, 12 Apr 2023 16:00:27 +0300
In-Reply-To: <56831ba4f216daee09b3d9c9a7deaf5810cade34.camel@realtek.com>
        (Ping-Ke Shih's message of "Tue, 11 Apr 2023 13:01:36 +0000")
Message-ID: <8735558dck.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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