linux-kernel - Re: [PATCH V4 2/2] cgroup/rstat: Avoid thundering herd problem by kswapd across NUMA nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJD7tkYX9OaAyWg=L_5v7GaKtKmptPpMGJh7Org5tcY4D-YnCw@mail.gmail.com>
Date: Tue, 16 Jul 2024 14:54:12 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: Shakeel Butt <shakeel.butt@...ux.dev>, tj@...nel.org, cgroups@...r.kernel.org, 
	hannes@...xchg.org, lizefan.x@...edance.com, longman@...hat.com, 
	kernel-team@...udflare.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 2/2] cgroup/rstat: Avoid thundering herd problem by
 kswapd across NUMA nodes

On Mon, Jul 8, 2024 at 8:26 AM Jesper Dangaard Brouer <hawk@...nel.org> wrote:
>
>
> On 28/06/2024 11.39, Jesper Dangaard Brouer wrote:
> >
> >
> > On 28/06/2024 01.34, Shakeel Butt wrote:
> >> On Thu, Jun 27, 2024 at 11:18:56PM GMT, Jesper Dangaard Brouer wrote:
> >>> Avoid lock contention on the global cgroup rstat lock caused by kswapd
> >>> starting on all NUMA nodes simultaneously. At Cloudflare, we observed
> >>> massive issues due to kswapd and the specific mem_cgroup_flush_stats()
> >>> call inlined in shrink_node, which takes the rstat lock.
> >>>
> [...]
> >>>   static void cgroup_base_stat_flush(struct cgroup *cgrp, int cpu);
> >>> @@ -312,6 +315,45 @@ static inline void __cgroup_rstat_unlock(struct
> >>> cgroup *cgrp, int cpu_in_loop)
> >>>       spin_unlock_irq(&cgroup_rstat_lock);
> >>>   }
> >>> +#define MAX_WAIT    msecs_to_jiffies(100)
> >>> +/* Trylock helper that also checks for on ongoing flusher */
> >>> +static bool cgroup_rstat_trylock_flusher(struct cgroup *cgrp)
> >>> +{
> >>> +    bool locked = __cgroup_rstat_trylock(cgrp, -1);
> >>> +    if (!locked) {
> >>> +        struct cgroup *cgrp_ongoing;
> >>> +
> >>> +        /* Lock is contended, lets check if ongoing flusher is already
> >>> +         * taking care of this, if we are a descendant.
> >>> +         */
> >>> +        cgrp_ongoing = READ_ONCE(cgrp_rstat_ongoing_flusher);
> >>> +        if (cgrp_ongoing && cgroup_is_descendant(cgrp, cgrp_ongoing)) {
> >>
> >> I wonder if READ_ONCE() and cgroup_is_descendant() needs to happen
> >> within in rcu section. On a preemptable kernel, let's say we got
> >> preempted in between them, the flusher was unrelated and got freed
> >> before we get the CPU. In that case we are accessing freed memory.
> >>
> >
> > I have to think about this some more.
> >
>
> I don't think this is necessary. We are now waiting (for completion) and
> not skipping flush, because as part of take down function
> cgroup_rstat_exit() is called, which will call cgroup_rstat_flush().
>
>
>   void cgroup_rstat_exit(struct cgroup *cgrp)
>   {
>         int cpu;
>         cgroup_rstat_flush(cgrp);
>
>

Sorry for the late response, I was traveling for a bit. I will take a
look at your most recent version shortly. But I do have a comment
here.

I don't see how this addresses Shakeel's concern. IIUC, if the cgroup
was freed after READ_ONCE() (and cgroup_rstat_flush() was called),
then cgroup_is_descendant() will access freed memory. We are not
holding the lock here so we are not preventing cgroup_rstat_flush()
from being called for the freed cgroup, right?