On Thu, Oct 03, 2024 at 02:58:19AM +0800, Kairui Song wrote:
> On Wed, Oct 2, 2024 at 7:28 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> >
> > On Wed, Oct 02, 2024 at 02:25:34PM +0300, Dan Carpenter wrote:
> > > On Wed, Oct 02, 2024 at 02:24:20PM +0300, Dan Carpenter wrote:
> > > > Let's add Kairui Song to the CC list.
> > > >
> > > > One simple thing is that we should add a READ_ONCE() to the comparison. Naresh,
> > > > could you test the attached diff? I don't know that it will fix it but it's
> > > > worth checking the easy stuff first.
> > > >
> > >
> > > Actually that's not right. Let me write a different patch.
> >
> > Try this one.
> >
> > regards,
> > dan carpenter
> >
> > diff --git a/mm/list_lru.c b/mm/list_lru.c
> > index 79c2d21504a2..2c429578ed31 100644
> > --- a/mm/list_lru.c
> > +++ b/mm/list_lru.c
> > @@ -65,6 +65,7 @@ lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
> > bool irq, bool skip_empty)
> > {
> > struct list_lru_one *l;
> > + long nr_items;
> > rcu_read_lock();
> > again:
> > l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg));
> > @@ -73,8 +74,9 @@ lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
> > spin_lock_irq(&l->lock);
> > else
> > spin_lock(&l->lock);
> > - if (likely(READ_ONCE(l->nr_items) != LONG_MIN)) {
> > - WARN_ON(l->nr_items < 0);
> > + nr_items = READ_ONCE(l->nr_items);
> > + if (likely(nr_items != LONG_MIN)) {
> > + WARN_ON(nr_items < 0);
> > rcu_read_unlock();
> > return l;
> > }
> >
>
> Thanks. The warning is a new added sanity check, I'm not sure if this
> WARN_ON triggered by an existing list_lru leak or if it's a new issue.
>
> And unfortunately so far I can't reproduce it locally on my ARM
> machine, it should be easily reproducible according to the
> description. And if the WARN only triggered once, and only during
> boot, mayce some static data wasn't initialized correctly?
I have a config where it printed twice and the second time wasn't during boot.
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20241009/testrun/25363339/suite/boot/test/gcc-13-lkftconfig-rcutorture/log
> Or the enablement of memcg caused some list_lru leak
> (mem_cgroup_from_slab_obj changed from returning NULL to returning
> actual memcg, so a item added to rootcg before will be attempt removed
> from actual memcg, seems a real race). If it's the latter case, then
> it's an existing issue caught by the new sanity check.
>
> The READ_ONCE patch may be worth trying, I'll also try to do more
> debugging on this and try to send a fix later.
The READ_ONCE() patch *seemed* to work, but the bug is intermittent so maybe it
just changed the timing or something. Still, I feel from a correctness
perspective the READ_ONCE() thing is probably correct, right?
regards,
dan carpenter
