Re: [PATCH v3 2/5] mm: kmem: add direct objcg pointer to task_struct

[Shakeel Butt <shakeelb@xxxxxxxxxx>]

On Wed, Oct 18, 2023 at 2:52 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
[...]
> >
> > +static struct obj_cgroup *current_objcg_update(void)
> > +{
> > +     struct mem_cgroup *memcg;
> > +     struct obj_cgroup *old, *objcg = NULL;
> > +
> > +     do {
> > +             /* Atomically drop the update bit. */
> > +             old = xchg(&current->objcg, NULL);
> > +             if (old) {
> > +                     old = (struct obj_cgroup *)
> > +                             ((unsigned long)old & ~CURRENT_OBJCG_UPDATE_FLAG);
> > +                     if (old)
> > +                             obj_cgroup_put(old);
> > +
> > +                     old = NULL;
> > +             }
> > +
> > +             /* Obtain the new objcg pointer. */
> > +             rcu_read_lock();
> > +             memcg = mem_cgroup_from_task(current);
> > +             /*
> > +              * The current task can be asynchronously moved to another
> > +              * memcg and the previous memcg can be offlined. So let's
> > +              * get the memcg pointer and try get a reference to objcg
> > +              * under a rcu read lock.
> > +              */
> > +             for (; memcg != root_mem_cgroup; memcg = parent_mem_cgroup(memcg)) {
> > +                     objcg = rcu_dereference(memcg->objcg);
> > +                     if (likely(objcg && obj_cgroup_tryget(objcg)))
>
> So IIUC here we increase objcg refcount.
>
> > +                             break;
> > +                     objcg = NULL;
> > +             }
> > +             rcu_read_unlock();
> > +
> > +             /*
> > +              * Try set up a new objcg pointer atomically. If it
> > +              * fails, it means the update flag was set concurrently, so
> > +              * the whole procedure should be repeated.
> > +              */
> > +     } while (!try_cmpxchg(&current->objcg, &old, objcg));
>
> And if this fails we throw objcg away and try again, but we should do
> obj_cgroup_put(objcg) first, as otherwise it would cause a leak?
>

Indeed there is a reference leak here.