On Wed, Jul 17, 2024 at 12:46 AM Jesper Dangaard Brouer <hawk@xxxxxxxxxx> wrote:
>
>
>
> On 16/07/2024 23.54, Yosry Ahmed wrote:
> > On Mon, Jul 8, 2024 at 8:26 AM Jesper Dangaard Brouer <hawk@xxxxxxxxxx> 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?
>
> If we go back to only allowing root-cgroup to be ongoing-flusher, then
> we could do a cgroup_rstat_flush(root) in cgroup_rstat_exit() to be sure
> nothing is left waiting for completion scheme. Right?
I am still not sure I understand how this helps.
We still need to call cgroup_is_descendant() because in cgroup v1 we
may have multiple root cgroups, right?
So it is still possible that the cgroup is freed after READ_ONCE() and
cgroup_is_descendant() accesses freed memory. Unless of course we have
other guarantees that the root cgroups will not go away.
Since at this point we are not holding the rstat lock, or actually
waiting for the ongoing flush (yet), I don't see how any
cgroup_rstat_flush() calls in the cgroup exit paths will help.
I actually think RCU may not help either for non-root cgroups, because
we call cgroup_rstat_flush() in cgroup_rstat_exit(), which is called
*after* the RCU grace period, and the cgroup is freed right away after
that. We may need to replace kfree(cgrp) with kfree_rcu(cgrp) in
css_free_rwork_fn().
>
> IMHO the code is getting too complicated with sub-cgroup's as ongoing
> flushers which also required having 'completion' queues per cgroup.
> We should go back to only doing this for the root-cgroup.
Because of multiple root cgroups in cgroup v1, we may still need that
anyway, right?
Please let me know if I am missing something.
