Re: Abnormal values show up in /proc/allocinfo

["David Wang" <00107082@163.com>]

Hi, 


At 2024-11-26 09:14:50, "David Wang" <00107082@163.com> wrote:
>
>Hi, 
>
>
>
>At 2024-11-26 04:31:39, "Suren Baghdasaryan" <surenb@google.com> wrote:
>>
>>Hi David,
>>Could you please check if you have this fix:
>>
>>ed265529d39a "mm/codetag: fix arg in pgalloc_tag_copy alloc_tag_sub"
>>
>>It was merged after v6.12-rc6 and it fixes an accounting bug inside
>>pgalloc_tag_copy(), which is used during compaction.
>>Thanks,
>>Suren.
>>
>>
>>https://lore.kernel.org/all/20240906042108.1150526-3-yuzhao@google.com/
>>https://lore.kernel.org/all/20241022232440.334820-1-souravpanda@google.com/
>
>
>No, ed265529d39a is not in 6.12.0; It is now in Linus' tree.
>I will pull the code and make some tests.
>
>Will update later.


I build a kernel based on a tree with top commit 9f16d5e6f220, 
no abnormal value observed for compaction_alloc:

$ sudo cat /proc/allocinfo  | grep compaction_alloc
           0        0 mm/compaction.c:1880 func:compaction_alloc 250098
(The last column is accumulative call counters I patched with my system, meaning compaction_alloc do happen)


But,  still got underflowed values:

       -4096 18446744073709551615 mm/filemap.c:3788 func:do_read_cache_folio 18 
   -86241280 18446744073709530561 mm/filemap.c:1951 func:__filemap_get_folio 8691015 

Finally a procedure to reproduce it:  (thanks for the tip about compact_memory)
1. populate file caches, e.g. grep something in kernel source.
2. echo 1 >/proc/sys/vm/compact_memory
3. echo 3 > /proc/sys/vm/drop_caches

There would be negative values show up.
A simple python script to check abnormal values:

with open("/proc/allocinfo") as f:
    for l in f:
        try:
            vs  = l.split()
            v1, v2 = int(vs[0]), int(vs[1])
            if v1<0 or v2<0 or (v1==0 and v2!=0) or (v1!=0 and v2==0): print l,
        except: pass


Most likely, memory release is accounted *twice* for those memory.
Reading through the code, I feel the most suspected code would be:
    clear_page_tag_ref(&old->page);
This line of code may not work as expected.

The code in pgalloc_tag_copy involves too many low level plumbing details, I think it 
is simpler to just implement a *swap* logic: just swap the tags.

I made following changes and it works, no abnormal values detected, so far.
(pgalloc_tag_copy should be renamed to pgalloc_tag_swap)


diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
index 2414a7ee7ec7..7d6d1015f4b1 100644
--- a/lib/alloc_tag.c
+++ b/lib/alloc_tag.c
@@ -191,24 +192,30 @@ void pgalloc_tag_split(struct folio *folio, int old_order, int new_order)
 
 void pgalloc_tag_copy(struct folio *new, struct folio *old)
 {
-	union pgtag_ref_handle handle;
-	union codetag_ref ref;
-	struct alloc_tag *tag;
+	union pgtag_ref_handle handle_new, handle_old;
+	union codetag_ref ref_new, ref_old;
+	struct alloc_tag *tag_new, *tag_old;
 
-	tag = pgalloc_tag_get(&old->page);
-	if (!tag)
+	tag_old = pgalloc_tag_get(&old->page);
+	if (!tag_old)
+		return;
+	tag_new = pgalloc_tag_get(&new->page);
+	if (!tag_new)
 		return;
 
-	if (!get_page_tag_ref(&new->page, &ref, &handle))
+	if (!get_page_tag_ref(&old->page, &ref_old, &handle_old))
+		return;
+	if (!get_page_tag_ref(&new->page, &ref_new, &handle_new))
 		return;
 
-	/* Clear the old ref to the original allocation tag. */
-	clear_page_tag_ref(&old->page);
-	/* Decrement the counters of the tag on get_new_folio. */
-	alloc_tag_sub(&ref, folio_size(new));
-	__alloc_tag_ref_set(&ref, tag);
-	update_page_tag_ref(handle, &ref);
-	put_page_tag_ref(handle);
+	/* swap tag */
+	__alloc_tag_ref_set(&ref_new, tag_old);
+	update_page_tag_ref(handle_new, &ref_new);
+	put_page_tag_ref(handle_new);
+
+	__alloc_tag_ref_set(&ref_old, tag_new);
+	update_page_tag_ref(handle_old, &ref_old);
+	put_page_tag_ref(handle_old);
 }
 
 static void shutdown_mem_profiling(bool remove_file)



FYI
David


>
>Thanks~
>David
>
>>
>>> > >Thanks,
>>> > >Suren.
>>> > >
>>> > >>
>>> > >>
>>> > >> Thanks
>>> > >> David
>>> > >>
>>> > >>
>>> > >>
>>> > >>