Hi Liuye,
On Fri, Sep 5, 2025 at 8:23 PM liuye <[email protected]> wrote:
>
>
> 在 2025/9/5 14:20, Tao Liu 写道:
> > Hi liuye,
> >
> > On Fri, Sep 5, 2025 at 12:29 AM <[email protected]> wrote:
> >> From 33e22a832bdf79b032dd9d4ceac53fead5c2f552 Mon Sep 17 00:00:00 2001
> >> From: Ye Liu <[email protected]>
> >> 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.
Sorry I still think the renaming of the local variable is not needed.
If you look into crash commit: 5f390ed811 and memory.c:
843 MEMBER_OFFSET_INIT(kmem_cache_cpu_page,
"kmem_cache_cpu", "page");
844 if (INVALID_MEMBER(kmem_cache_cpu_page))
845
MEMBER_OFFSET_INIT(kmem_cache_cpu_page, "kmem_cache_cpu", "slab");
Because member "page" only exist for old kernels, for recent kernel
the "page" is replaced by "slab":
old kernel:
struct kmem_cache_cpu {
struct page *page;
}
recent kernel:
struct kmem_cache_cpu {
struct slab *slab;
}
So the variable "kmem_cache_cpu_page" can both mean the offset of
"page ptr" and "slab ptr".
With this, the rename of the local variable from "cpu_slab_ptr" to
"cpu_slab_page_ptr" seems only reasonable for "page ptr" case, but not
for "slab ptr" case. As you know that the kernel structures always
change, but crash has to keep them all in order to make it compatible
with different kernel versions. In different kernel versions, the same
variable might mean something slightly different, and we cannot pin
one name for a variable to make all kernel versions happy. So a
context of that variable is more reliable than naming.
Thanks,
Tao Liu
>
> 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 <[email protected]>
> >> ---
> >> 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 -- [email protected]
> >> To unsubscribe send an email to [email protected]
> >> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> >> Contribution Guidelines: https://github.com/crash-utility/crash/wiki
>
> --
> Thanks,
> Ye Liu
>
--
Crash-utility mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
