[Crash-utility] Re: [PATCH] mem: rename variable for clarity in get_kmem_cache_slub_data()

在 2025/9/5 14:20, Tao Liu 写道: > Hi liuye, > > On Fri, Sep 5, 2025 at 12:29 AM <liuye(a)kylinos.cn&gt; wrote: >> From 33e22a832bdf79b032dd9d4ceac53fead5c2f552 Mon Sep 17 00:00:00 2001 >> From: Ye Liu <liuye(a)kylinos.cn&gt; >> Date: Wed, 3 Sep 2025 15:18:57 +0800 >> Subject: [PATCH] mem: rename variable for clarity in >> get_kmem_cache_slub_data() >> >> Rename 'cpu_slab_ptr' to 'cpu_slab_page_ptr' to better reflect that >> it holds a pointer to a page structure rather than a generic slab pointer. >> This improves code readability and consistency with related functions. >> >> No functional changes. > Frankly I don't think the rename is necessary: > > 1) It is a local variable which only exist within function > get_kmem_cache_slub_data(), the context relating to the r/w of the > variable is simple and clear, such as: > > cpu_slab_ptr = ULONG(si->cache_buf + > OFFSET(kmem_cache_cpu_slab) + > OFFSET(kmem_cache_cpu_page)); > > Easy for people to understand cpu_slab_ptr comes from kmem_cache -> > slab -> page. > > 2) The cpu_slab_ptr variable have different meaning in different stage, e.g: > > stage1: > cpu_slab_ptr = ULONG(si->cache_buf + > OFFSET(kmem_cache_cpu_slab) + (sizeof(void *)*cpu)); > > readmem(cpu_slab_ptr + OFFSET(kmem_cache_cpu_page), > KVADDR, &page, sizeof(void *), > > stage2: > cpu_slab_ptr = page; > > your cpu_slab_page_ptr renaming only makes sense for the stage2. To > me, the cpu_slab_ptr acts like a tmp ptr in this case, so it doesn't > matter to me what name it should be. To avoid necessary patches, I'd > prefer not to accept this patch unless you have more persuasive > reasons. > > Thanks, > Tao Liu Hi Tao, Thank you for your feedback on the patch. I understand your perspective about the variable being local and serving as a temporary pointer. Let me explain my reasoning for proposing this change: Semantic clarity: While cpu_slab_ptr does work as a temporary pointer, the specific nature of what it points to (a page structure) is important for understanding the subsequent operations. The page_to_nid() call and the OFFSET(page_inuse) access both operate on page structures, not generic slab pointers. Consistency with related functions: The get_cpu_slab_ptr() function actually returns a pointer to a page structure (as evidenced by its use of OFFSET(kmem_cache_cpu_page) internally). Renaming the variable to cpu_slab_page_ptr creates better consistency between the function's purpose and how its return value is used. Code documentation: The rename serves as implicit documentation, making it immediately clear to readers that we're working with a page structure rather than having to trace through the code to understand what type of pointer we're dealing with. Maintenance benefit: While the context is clear now, this clarification could help future developers who might be less familiar with this specific code path understand the data flow more quickly. I agree that the variable serves as a temporary pointer, but I believe the additional specificity in the naming helps convey the architectural intent more clearly - that we're specifically retrieving and working with the page structure portion of the CPU slab. I hadn't looked at the source code before this, but I encountered an issue (https://github.com/crash-utility/crash/issues/218) that compelled me to look. Before replying to this email, I took a closer look at some of the source code. Regarding the use of the get_cpu_slab_ptr function, the variable receiving the return value is always named cpu_slab_ptr, even though it has two meanings. This may be a coding style or a legacy.

That said, I respect your decision as maintainer. If you still feel this change isn't necessary, I'm happy to withdraw the patch. >> Signed-off-by: Ye Liu <liuye(a)kylinos.cn&gt; >> --- >> memory.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/memory.c b/memory.c >> index 400d31a04cd4..be5c590003a8 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -19426,7 +19426,7 @@ get_kmem_cache_slub_data(long cmd, struct meminfo *si) >> { >> int i, n, node; >> ulong total_objects, total_slabs, free_objects; >> - ulong cpu_slab_ptr, node_ptr, cpu_freelist, orig_slab; >> + ulong cpu_slab_page_ptr, node_ptr, cpu_freelist, orig_slab; >> ulong node_nr_partial, node_nr_slabs, node_total_objects; >> int full_slabs, objects, node_total_avail; >> long p; >> @@ -19445,12 +19445,12 @@ get_kmem_cache_slub_data(long cmd, struct meminfo *si) >> node_total_avail = VALID_MEMBER(kmem_cache_node_total_objects) ? TRUE : FALSE; >> >> for (i = 0; i < kt->cpus; i++) { >> - cpu_slab_ptr = get_cpu_slab_ptr(si, i, &cpu_freelist); >> + cpu_slab_page_ptr = get_cpu_slab_ptr(si, i, &cpu_freelist); >> >> - if (!cpu_slab_ptr) >> + if (!cpu_slab_page_ptr) >> continue; >> >> - if ((node = page_to_nid(cpu_slab_ptr)) < 0) >> + if ((node = page_to_nid(cpu_slab_page_ptr)) < 0) >> goto bailout; >> >> switch (cmd) >> @@ -19458,15 +19458,15 @@ get_kmem_cache_slub_data(long cmd, struct meminfo *si) >> case GET_SLUB_OBJECTS: { >> /* For better error report, set cur slab to si->slab. */ >> orig_slab = si->slab; >> - si->slab = cpu_slab_ptr; >> + si->slab = cpu_slab_page_ptr; >> >> - if (!readmem(cpu_slab_ptr + OFFSET(page_inuse), >> + if (!readmem(cpu_slab_page_ptr + OFFSET(page_inuse), >> KVADDR, &inuse, sizeof(short), >> "page inuse", RETURN_ON_ERROR)) { >> si->slab = orig_slab; >> return FALSE; >> } >> - objects = slub_page_objects(si, cpu_slab_ptr); >> + objects = slub_page_objects(si, cpu_slab_page_ptr); >> if (!objects) { >> si->slab = orig_slab; >> return FALSE; >> -- >> 2.43.0 >> -- >> Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io >> To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io >> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ >> Contribution Guidelines: https://github.com/crash-utility/crash/wiki -- Thanks, Ye Liu