Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20231016221900.4031141-1-roman.gushchin@linux.dev>
 <20231016221900.4031141-3-roman.gushchin@linux.dev> <d698b8d0-1697-e336-bccb-592e633e8b98@suse.cz>
In-Reply-To: <d698b8d0-1697-e336-bccb-592e633e8b98@suse.cz>
From:   Shakeel Butt <shakeelb@google.com>
Date:   Wed, 18 Oct 2023 08:29:14 -0700
Message-ID: <CALvZod5bxWQ8nwxyxv=ySN-75FEgtCKWussyNyFeDadURQ2XYw@mail.gmail.com>
Subject: Re: [PATCH v3 2/5] mm: kmem: add direct objcg pointer to task_struct
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     Roman Gushchin <roman.gushchin@linux.dev>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Muchun Song <muchun.song@linux.dev>,
        Dennis Zhou <dennis@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Naresh Kamboju <naresh.kamboju@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 18, 2023 at 2:52=E2=80=AFAM Vlastimil Babka <vbabka@suse.cz> wr=
ote:
[...]
> >
> > +static struct obj_cgroup *current_objcg_update(void)
> > +{
> > +     struct mem_cgroup *memcg;
> > +     struct obj_cgroup *old, *objcg =3D NULL;
> > +
> > +     do {
> > +             /* Atomically drop the update bit. */
> > +             old =3D xchg(&current->objcg, NULL);
> > +             if (old) {
> > +                     old =3D (struct obj_cgroup *)
> > +                             ((unsigned long)old & ~CURRENT_OBJCG_UPDA=
TE_FLAG);
> > +                     if (old)
> > +                             obj_cgroup_put(old);
> > +
> > +                     old =3D NULL;
> > +             }
> > +
> > +             /* Obtain the new objcg pointer. */
> > +             rcu_read_lock();
> > +             memcg =3D mem_cgroup_from_task(current);
> > +             /*
> > +              * The current task can be asynchronously moved to anothe=
r
> > +              * 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 !=3D root_mem_cgroup; memcg =3D parent_mem_c=
group(memcg)) {
> > +                     objcg =3D rcu_dereference(memcg->objcg);
> > +                     if (likely(objcg && obj_cgroup_tryget(objcg)))
>
> So IIUC here we increase objcg refcount.
>
> > +                             break;
> > +                     objcg =3D 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.