On Thu, Oct 10, 2024 at 12:51 AM Dan Carpenter dan.carpenter@linaro.org 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@linaro.org 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/tes...
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.