[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <D2BDE4C4-48DB-4813-9608-F672896E0121@gmail.com>
Date: Thu, 19 Sep 2024 21:57:12 +0800
From: Alan Huang <mmpgouride@...il.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Lai Jiangshan <jiangshanlai@...il.com>,
LKML <linux-kernel@...r.kernel.org>,
RCU <rcu@...r.kernel.org>,
linux-mm@...ck.org,
lkmm@...ts.linux.dev,
"Paul E. McKenney" <paulmck@...nel.org>,
Frederic Weisbecker <frederic@...nel.org>,
Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
Joel Fernandes <joel@...lfernandes.org>,
Josh Triplett <josh@...htriplett.org>,
"Uladzislau Rezki (Sony)" <urezki@...il.com>,
Steven Rostedt <rostedt@...dmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Zqiang <qiang.zhang1211@...il.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Will Deacon <will@...nel.org>,
Waiman Long <longman@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Kent Overstreet <kent.overstreet@...il.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Vlastimil Babka <vbabka@...e.cz>,
maged.michael@...il.com,
Neeraj upadhyay <neeraj.upadhyay@....com>
Subject: Re: [RFC PATCH 1/4] hazptr: Add initial implementation of hazard
pointers
2024年9月19日 15:10,Boqun Feng <boqun.feng@...il.com> wrote:
>
> On Thu, Sep 19, 2024 at 02:39:13PM +0800, Lai Jiangshan wrote:
>> On Tue, Sep 17, 2024 at 10:34 PM Boqun Feng <boqun.feng@...il.com> wrote:
>>
>>> +static void hazptr_context_snap_readers_locked(struct hazptr_reader_tree *tree,
>>> + struct hazptr_context *hzcp)
>>> +{
>>> + lockdep_assert_held(hzcp->lock);
>>> +
>>> + for (int i = 0; i < HAZPTR_SLOT_PER_CTX; i++) {
>>> + /*
>>> + * Pairs with smp_store_release() in hazptr_{clear,free}().
>>> + *
>>> + * Ensure
>>> + *
>>> + * <reader> <updater>
>>> + *
>>> + * [access protected pointers]
>>> + * hazptr_clear();
>>> + * smp_store_release()
>>> + * // in reader scan.
>>> + * smp_load_acquire(); // is null or unused.
>>> + * [run callbacks] // all accesses from
>>> + * // reader must be
>>> + * // observed.
>>> + */
>>> + hazptr_t val = smp_load_acquire(&hzcp->slots[i]);
>>> +
>>> + if (!is_null_or_unused(val)) {
>>> + struct hazptr_slot_snap *snap = &hzcp->snaps[i];
>>> +
>>> + // Already in the tree, need to remove first.
>>> + if (!is_null_or_unused(snap->slot)) {
>>> + reader_del(tree, snap);
>>> + }
>>> + snap->slot = val;
>>> + reader_add(tree, snap);
>>> + }
>>> + }
>>> +}
>>
>> Hello
>>
>> I'm curious about whether there are any possible memory leaks here.
>>
>> It seems that call_hazptr() never frees the memory until the slot is
>> set to another valid value.
>>
>> In the code here, the snap is not deleted when hzcp->snaps[i] is null/unused
>> and snap->slot is not which I think it should be.
>>
>> And it can cause unneeded deletion and addition of the snap if the slot
>> value is unchanged.
>>
>
> I think you're right. (Although the node will be eventually deleted at
> cleanup_hazptr_context(), however there could be a long-live
> hazptr_context). It should be:
>
> hazptr_t val = smp_load_acquire(&hzcp->slots[i]);
> struct hazptr_slot_snap *snap = &hzcp->snaps[i];
>
> if (val != snap->slot) { // val changed, need to update the tree node.
> // Already in the tree, need to remove first.
> if (!is_null_or_unused(snap->slot)) {
> reader_del(tree, snap);
> }
>
> // use the latest snapshot.
> snap->slot = val;
>
> // Add it into tree if there is a reader
> if (!is_null_or_unused(val))
> reader_add(tree, snap);
> }
It seems like that two different hazptr_context can’t be used to protect the same pointer?
Otherwise the following can happen?
thread1 thread2 thread3(worker) thread4
hazptr_tryprotect(hzp1, ptr1) hazptr_tryprotect(hzp2, ptr1)
add ptr1 to tree
hazptr_clear(hzp1)
hazptr_tryprotect(hzp1, ptr2)
delete ptr1 from tree unpub ptr1
call_hazptr(ptr1)
oops: invoke ptr1's callback
Or am I missing something?
>
> Regards,
> Boqun
>
>> I'm not so sure...
>>
>> Thanks
>> Lai
Powered by blists - more mailing lists