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.weiy...@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.f...@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.weiy...@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.weiy...@gmail.com> >> >>>> Link: >> >>>> https://lore.kernel.org/r/20250101082306.10404-1-richard.weiy...@gmail.com >> >>>> Signed-off-by: Boqun Feng <boqun.f...@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
