在 2025/9/8 13:47, Tao Liu 写道:
> Hi Liuye,
>
> On Fri, Sep 5, 2025 at 8:23 PM liuye <li...@kylinos.cn> wrote:
>>
>> 在 2025/9/5 14:20, Tao Liu 写道:
>>> Hi liuye,
>>>
>>> On Fri, Sep 5, 2025 at 12:29 AM <li...@kylinos.cn> wrote:
>>>> From 33e22a832bdf79b032dd9d4ceac53fead5c2f552 Mon Sep 17 00:00:00 2001
>>>> From: Ye Liu <li...@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.
> 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;
> }
I ignored this patch before, and according to the current situation, it is
really not suitable to modify it.
Thanks,
Ye Liu
>
> 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 <li...@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@lists.crash-utility.osci.io
>>>> To unsubscribe send an email to devel-le...@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
>>
--
Thanks,
Ye Liu
--
Crash-utility mailing list -- devel@lists.crash-utility.osci.io
To unsubscribe send an email to devel-le...@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
