[<prev] [next>] [day] [month] [year] [list]
Message-ID: <87frgmai9f.fsf@oracle.com>
Date: Thu, 29 May 2025 20:29:16 -0700
From: Stephen Brennan <stephen.s.brennan@...cle.com>
To: Ye Liu <ye.liu@...ux.dev>, akpm@...ux-foundation.org
Cc: linux-debuggers@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, linux-toolchains@...r.kernel.org,
osandov@...ndov.com, paulmck@...nel.org, sweettea-kernel@...miny.me,
liuye@...inos.cn, fweimer@...hat.com, sj@...nel.org
Subject: Re: [PATCH v4] tools/mm: Add script to display page state for a
given PID and VADDR
Ye Liu <ye.liu@...ux.dev> writes:
>>> +import argparse
>>> +from drgn import Object, FaultError
>>> +from drgn.helpers.linux import find_task, follow_page, page_size
>>> +from drgn.helpers.linux.mm import (
>>> + decode_page_flags, page_to_pfn, page_to_phys, page_to_virt, vma_find,
>>> + PageSlab, PageCompound, PageHead, PageTail, compound_head, compound_order, compound_nr
>>> +)
>>> +from drgn.helpers.linux.cgroup import cgroup_name, cgroup_path
>> Anything in "drgn.helpers.linux.*" can be imported from
>> "drgn.helpers.linux" instead, which would help if any helper moved
>> around from one module to another. I've recently started preferring
>> that, but I don't know if it's a huge improvement. EG:
>>
>> from drgn.helpers.linux import (
>> PageCompound, PageHead, PageSlab, PageTail, cgroup_name,
>> cgroup_path, compound_head, compound_nr, compound_order,
>> decode_page_flags, find_task, follow_page, page_size, page_to_pfn,
>> page_to_phys, page_to_virt, vma_find,
>> )
>>
>> Again, not sure it improves anything :)
> Thanks for the suggestion! After considering the trade-offs, I prefer
> keeping the current imports for clarity:
> Readability: Explicit module paths (e.g., mm/, cgroup/) make helper
> origins clearer.
> Debugging: Functional grouping helps when analyzing code.
> Both styles work, but the current approach aligns better with drgn’s
> documentation and our workflow. Happy to revisit if needs change.
Sounds good!
>>> +def show_page_state(page, addr, mm, pid, task):
>>> + """Display detailed information about a page."""
>>> + try:
>>> + print(f'PID: {pid} Comm: {task.comm.string_().decode()} mm: {hex(mm)}')
>>> + try:
>>> + print(format_page_data(prog.read(page.value_(), 64)))
>> Rather than hard-code the size of struct page, you can use sizeof(page).
>> And in fact, all drgn Objects have a .bytes_() that will just give you
>> the bytes of the object directly, which would even avoid the sizeof().
> I didn't find the .bytes_() method. Can you give an example?
> I used prog.type("struct page").size instead.
You're right, it's "to_bytes_()", sorry:
>>> prog["slab_caches"]
(struct list_head){
.next = (struct list_head *)0xffff9f604cbecd68,
.prev = (struct list_head *)0xffff9f6040042068,
}
>>> prog["slab_caches"].to_bytes_()
b'h\xcd\xbeL`\x9f\xff\xffh \x04@`\x9f\xff\xff'
https://drgn.readthedocs.io/en/latest/api_reference.html#drgn.Object.to_bytes_
But stick with sizeof() (or prog.type("struct page").size), that way you
can use Program.read_word() as mentioned above.
>>> +def main():
>>> + """Main function to parse arguments and display page state."""
>>> + parser = argparse.ArgumentParser(description=DESC, formatter_class=argparse.RawTextHelpFormatter)
>>> + parser.add_argument('pid', metavar='PID', type=int, help='Target process ID (PID)')
>>> + parser.add_argument('vaddr', metavar='VADDR', type=str, help='Target virtual address in hexadecimal format (e.g., 0x7fff1234abcd)')
>>> + args = parser.parse_args()
>>> +
>>> + try:
>>> + vaddr = int(args.vaddr, 16)
>>> + except ValueError:
>>> + print(f"Error: Invalid virtual address format: {args.vaddr}")
>>> + return
>> I find it quite useful to replace things like this with:
>>
>> sys.exit(f"Error: Invalid virtual address format: {args.vaddr}")
>>
>> Which will result in the script exiting with a non-zero exit code, and
>> it will print the message to stderr, rather than stdout. All while being
>> one line shorter, for the code golfers :)
> Agree, I can replace it in the main() function, but in other places,
> I prefer the script to continue running instead of exiting."
Yes definitely!
Regards,
Stephen
Powered by blists - more mailing lists