On Thu, Oct 10, 2024 at 12:51 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> 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?
>
Yes, the READ_ONCE fix is absolutely correct.
Not sure if it's possible in theory, that the compiler or CPU will use
the old value for the `WARN`, but use a new read value for the `if` above.
This READ_ONCE will prevent that from happening, if possible.
I think we should just merge the READ_ONCE fix, and see if any more
tests expose this issue again.
