Re: [PATCH] mm/memcontrol: stop resize loop if limit was changed again

From: Pavel Tikhomirov
Date: Thu Mar 21 2024 - 01:16:14 EST




On 21/03/2024 01:09, Waiman Long wrote:

On 3/20/24 06:03, Pavel Tikhomirov wrote:
In memory_max_write() we first set memcg->memory.max and only then
try to enforce it in loop. What if while we are in loop someone else
have changed memcg->memory.max but we are still trying to enforce
the old value? I believe this can lead to nasty consequence like getting
an oom on perfectly fine cgroup within it's limits or excess reclaim.

Concurrent write to the same cgroup control file is not possible as the underlying kernfs_open_file structure has a mutex that serialize access to the file. Concurrent write to different cgroup control files is possible, though.

Thanks for pointing this out, now I see it, in kernfs_fop_write_iter() we take of->mutex before ops->write() -> cgroup_file_write(). That means patch is not needed.


Cheers,
Longman


We also have exactly the same thing in memory_high_write().

So let's stop enforcing old limits if we already have a new ones.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@xxxxxxxxxxxxx>
---
  mm/memcontrol.c | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 61932c9215e7..81b303728491 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6769,6 +6769,9 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
          unsigned long nr_pages = page_counter_read(&memcg->memory);
          unsigned long reclaimed;
+        if (memcg->memory.high != high)
+            break;
+
          if (nr_pages <= high)
              break;
@@ -6817,6 +6820,9 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
      for (;;) {
          unsigned long nr_pages = page_counter_read(&memcg->memory);
+        if (memcg->memory.max != max)
+            break;
+
          if (nr_pages <= max)
              break;


--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.