Re: [Crash-utility] [PATCH] display MCNT and PRIVATE when using kmem -p

----- Original Message ----- > Hello Dave, > > I think it is necessary to see what is hided in the union where > _mapcount lies. However, as you said, it's hard to pick which fields are > more important than others when adding new items to "kmem -p". So I > think over using struct sub-command to show what I want. > > What if I change struct sub-command to this: > > 1. it can refer to anonymous members (e.g., page._mapcount) > 2. it can refer to submembers(e.g., page._count.counter) > 3. it can output easy-parsing format (using an option to specify), maybe > like 'kmem -p'. For example, > crash> struct page.flags,_count.counter -..< PAGE_list.txt > 1024 0 > 1024 1 > 1024 1 > 1024 1 > > After adding these features to struct sub-command, I guess it is more > easier to get information hiding in structs and parsing it. Before > implementing, I feel the necessity to ask you for some advices. So what > about these features? That would certainly be useful. The problem in getting that to work would be twofold: (1) handling a "member" request that has a "." in it. crash> struct page._count.counter ffffea0000000400 struct: invalid format: page._count.counter crash> (2) getting a successful return value from the call to arg_to_datatype() in cmd_datatype_common(). crash> struct page.private ffffea0000000400 struct: invalid data structure reference: page.private crash> And both of the above would require getting the get_member_data() function in gdb-7.3.1/gdb/symtab.c to dig out the information for anonymous members, which it currently does not do. In this part of get_member_data(), the requested member name string is searched for: for (i = 0; i< nfields; i++) { if (STREQ(req->member, nextfield->name)) { req->member_offset = nextfield->loc.bitpos; req->member_length = TYPE_LENGTH(nextfield->type); req->member_typecode = TYPE_CODE(nextfield->type); if ((req->member_typecode == TYPE_CODE_TYPEDEF)&& (typedef_type = check_typedef(nextfield->type))) req->member_length = TYPE_LENGTH(typedef_type); return; } nextfield++; } When get_member_data() walks through the page stucture, it does find the anonymous members above, but nextfield->name points to a NULL name string. That seems to be related to gdb's behavior when asking it to simply print a page structure, which is requested with a "ptype struct page" gdb command: crash> struct page struct page { long unsigned int flags; struct address_space *mapping; struct { union {...}; union {...}; }; struct list_head lru; union { long unsigned int private; spinlock_t ptl; struct kmem_cache *slab; struct page *first_page; }; } SIZE: 64 crash> I don't know how to get gdb to display the "full" structure declaration? Anyway, when given an internal request to display a page structure from memory, it does display them: crash> page ffffea0000000400 struct page { flags = 0, mapping = 0x0, { { index = 18446612132314288112, freelist = 0xffff880000010ff0 }, { counters = 4294967168, { { _mapcount = { counter = -128 }, { inuse = 65408, objects = 32767, frozen = 1 } }, _count = { counter = 0 } } } }, lru = { next = 0xffffea00000042a0, prev = 0xffffea00000005a0 }, { private = 1, ptl = { { rlock = { raw_lock = { slock = 1 } } } }, slab = 0x1, first_page = 0x1 } } crash> And because that works OK, the ANON_MEMBER_OFFSET_REQUEST() macro exists to handle cases for required offset_table entries. Here, if I put a debug printf each time though the member loop in get_member_data(), you'll see this: crash> page.slab ffffea0000000400 page -> flags page -> mapping page -> page -> lru page -> struct: invalid data structure reference: page.slab crash> which again, reflects what happens when a page struct is printed: crash> struct page struct page { long unsigned int flags; struct address_space *mapping; struct { union {...}; union {...}; }; struct list_head lru; union { long unsigned int private; spinlock_t ptl; struct kmem_cache *slab; struct page *first_page; }; } SIZE: 64 crash> So, anyway, I would presume that perhaps something could be applied to get_member_data() to and check out the fields with NULL name strings, i.e.: for (i = 0; i< nfields; i++) { if (STREQ(req->member, nextfield->name)) { req->member_offset = nextfield->loc.bitpos; req->member_length = TYPE_LENGTH(nextfield->type); req->member_typecode = TYPE_CODE(nextfield->type); if ((req->member_typecode == TYPE_CODE_TYPEDEF)&& (typedef_type = check_typedef(nextfield->type))) req->member_length = TYPE_LENGTH(typedef_type); return; } + if (strlen(nextfield->name) == 0) { + ... + } nextfield++; } And that section would have to somehow deal with members that are part of anonymous struct members (like "private"), as well as with anonymous members that are expressed with a "." in them (like "_mapcount.counter). I don't expect it will be very easy to accomplish. But it would be a desirable capability, so please be my guest! Thanks, Dave > At 2012-1-6 3:37, Dave Anderson wrote: >> I appreciate the effort, but I'm not sure whether it's worth changing >> it at this point, or whether it could be accomplished in a different >> manner. >> >> The primary purpose for "kmem -p" is to show the page structure >> address associated with each physical address in the system -- along >> with "basic information about each page". It's had those basic >> fields in it forever -- which BTW, fit into 80 columns. I prefer not >> to have command output exceed 80 columns unless it is impossible to >> predict the size of an output field. >> >> Anyway, the page structure definition keeps changing over time, more >> specifically the embedded anonymous structures contained within it, and >> the fields within the anonymous structs have multiple meanings. With >> your patch, the output becomes cluttered and hard to understand, especially >> due to the strange values that can be seen in the MCNT column when it's >> not a counter value, but rather a slab-page construct: >> >> union { >> atomic_t _mapcount; >> >> struct { >> unsigned inuse:16; >> unsigned objects:15; >> unsigned frozen:1; >> }; >> }; >> >> And so it's hard to pick which fields are more important than >> others, >> because it pretty much depends upon what's being debugged. You >> have >> picked the private field (which can have numerous meanings), but >> for >> example, there have been times in the past where I wanted to see >> the >> lru list_head contents. >> >> That all being said, your patch does have merit, but I wonder if >> there >> could be an alternate way of selecting or filtering what fields are >> displayed? > > > -- > -- > Regards > Qiao Nuohan > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility