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@xxxxxxxxx> 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);
}
Regards,
Boqun
> I'm not so sure...
>
> Thanks
> Lai
