Re: [PATCH v4] tools/mm: Add script to display page state for a given PID and VADDR

Ye Liu <[email protected]> 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