3:22 a.m.
在 2025/9/5 14:20, Tao Liu 写道:
Hi liuye,
On Fri, Sep 5, 2025 at 12:29 AM <liuye(a)kylinos.cn> wrote:
> From 33e22a832bdf79b032dd9d4ceac53fead5c2f552 Mon Sep 17 00:00:00 2001
> From: Ye Liu <liuye(a)kylinos.cn>
> 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>
> ---
> 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