Re: [PATCH] doc/RCU/listRCU: fix an example code snippets

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Feb 17, 2025 at 02:30:59PM -0800, Boqun Feng wrote:
>On Mon, Feb 17, 2025 at 09:18:42AM +0000, Wei Yang wrote:
>> On Mon, Feb 17, 2025 at 04:02:53PM +0800, Alan Huang wrote:
>> >On Feb 17, 2025, at 15:41, Wei Yang <richard.weiyang@gmail.com> wrote:
>> >> 
>> >> On Mon, Feb 17, 2025 at 10:22:53AM +0800, Alan Huang wrote:
>> >>> On Feb 17, 2025, at 10:12, Boqun Feng <boqun.feng@gmail.com> wrote:
>> >>>> 
>> >>>> Hi Wei,
>> >>>> 
>> >>>> The change loosk good to me, thanks!
>> >>>> 
>> >>>> I queued the patch for futher reviews and tests with some changes in the
>> >>>> commit log (for title formating and a bit more explanation), please see
>> >>>> below.
>> >>>> 
>> >>>> Regards,
>> >>>> Boqun
>> >>>> 
>> >>>> On Wed, Jan 01, 2025 at 08:23:06AM +0000, Wei Yang wrote:
>> >>>>> The example code for "Eliminating Stale Data" looks not correct:
>> >>>>> 
>> >>>>> * rcu_read_unlock() should put after kstrdup()
>> >>>>> * spin_unlock() should be called before return
>> >>>>> 
>> >>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> >>>> [...]
>> >>>> 
>> >>>> 
>> >>>> ------------------>8
>> >>>> Subject: [PATCH] doc/RCU/listRCU: Fix an example code snippets
>> >>>> 
>> >>>> The example code for "Eliminating Stale Data" looks not correct:
>> >>>> 
>> >>>> * rcu_read_unlock() should put after kstrdup(), because otherwise
>> >>>> entry may get freed while kstrdup() is being called.
>> >>>> 
>> >>>> * spin_unlock() should be called before return, otherwise the
>> >>>> function would return with the lock of the entry held.
>> >>>> 
>> >>>> Hence fix these.
>> >>>> 
>> >>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> >>>> Link: https://lore.kernel.org/r/20250101082306.10404-1-richard.weiyang@gmail.com
>> >>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> >>>> ---
>> >>>> Documentation/RCU/listRCU.rst | 3 ++-
>> >>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>>> 
>> >>>> diff --git a/Documentation/RCU/listRCU.rst b/Documentation/RCU/listRCU.rst
>> >>>> index ed5c9d8c9afe..8df50fcd69fd 100644
>> >>>> --- a/Documentation/RCU/listRCU.rst
>> >>>> +++ b/Documentation/RCU/listRCU.rst
>> >>>> @@ -348,9 +348,10 @@ to accomplish this would be to add a ``deleted`` flag and a ``lock`` spinlock to
>> >>>> rcu_read_unlock();
>> >>>> return AUDIT_BUILD_CONTEXT;
>> >>>> }
>> >>>> - rcu_read_unlock();
>> >>>> if (state == AUDIT_STATE_RECORD)
>> >>>> *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
>> >>>> + spin_unlock(&e->lock);
>> >>> 
>> >>> According to the above quick quiz, we should return with the lock held.
>> >>> 
>> >> 
>> >> Thanks, I think you have some reason.
>> >> 
>> >> If my understanding is correct, the example here is to emphasize we could
>> >> still access the value out of critical section but with spinlock held.
>> >
>> >This example is intended to highlight how we can eliminate stale data.
>> >
>> 
>> Yes, you are more accurate.
>> 
>> >> 
>> >> In current example, we don't return e(struct audit_entry) from
>> >> audit_filter_task(). So no one suppose to release the spinlock again. This
>> >> looks to be a mistake.
>> >
>> >Then the example code should return e instead. ( *key is also undefined)
>> >
>> 
>> So you prefer a version with e returned?
>> 
>> Boqun
>> 
>> What's your preference?
>> 
>
>Yeah, I think it make more sense with e returned, and you can add some
>comments at the return statement like:
>
>	return e; // as long as the lock of e is held, e is valid.
>
>, but feel free to use whatever you see fit.
>

Thanks, let me prepare one.

>Regards,
>Boqun

-- 
Wei Yang
Help you, Help me