Re: [Crash-utility] [PATCH] runq: make tasks in throttled cfs_rqs/rt_rqs displayed

----- Original Message ----- >>> >>> I worry about the large number of kernel structure.member declarations that the >>> command depends upon, because if just one of them changes, it breaks the command. >>> So it would be preferable if the "runq" command (with no arguments) would only use >>> a minimal set of structure.member offsets. > > I made two new offset-init functions for runq: > > static void cfs_rq_offset_init(void); > static void task_group_offset_init(void); One minor suggestion -- it's really not necessary to call MEMBER_EXISTS() prior to calling MEMBER_OFFSET_INIT(): diff --git a/kernel.c b/kernel.c index 45da48e..868d777 100755 --- a/kernel.c +++ b/kernel.c @@ -308,6 +308,12 @@ kernel_init() STRUCT_SIZE_INIT(prio_array, "prio_array"); MEMBER_OFFSET_INIT(rq_cfs, "rq", "cfs"); + if (MEMBER_EXISTS("task_group", "cfs_rq")) + MEMBER_OFFSET_INIT(task_group_cfs_rq, "task_group", "cfs_rq"); + if (MEMBER_EXISTS("task_group", "rt_rq")) + MEMBER_OFFSET_INIT(task_group_rt_rq, "task_group", "rt_rq"); + if (MEMBER_EXISTS("task_group", "parent")) + MEMBER_OFFSET_INIT(task_group_parent, "task_group", "parent"); You can simply just call MEMBER_OFFSET_INIT() the 3 times above. If the structure members don't exist, MEMBER_OFFSET_INIT() will just set the offsets to -1 (INVALID_OFFSET). The same thing applies in your task_group_offset_init() function: + if (MEMBER_EXISTS("task_group", "cfs_bandwidth")) { + MEMBER_OFFSET_INIT(task_group_cfs_bandwidth, + "task_group", "cfs_bandwidth"); + MEMBER_OFFSET_INIT(cfs_rq_throttled, "cfs_rq", + "throttled"); + } + + if (MEMBER_EXISTS("task_group", "rt_bandwidth")) { + MEMBER_OFFSET_INIT(task_group_rt_bandwidth, + "task_group", "rt_bandwidth"); + MEMBER_OFFSET_INIT(rt_rq_rt_throttled, "rt_rq", + "rt_throttled"); + } Sometimes it helps to call MEMBER_EXISTS() for clarity's sake, but it's actually kind of redundant.

>> >> And to follow up, I'm still running tests (and will do so overnight) on your latest >> patch, but I immediately see this on any 2.6.30, 2.6.31 or 2.6.32 kernel, and on >> some 2.6.36 and 2.6.38 kernels, where "runq -g" fails like this: >> >> crash> runq -g >> runq: invalid kernel virtual address: 0 type: "dentry" >> crash> >> > > As you have pointed in another mail, this is fixed in patch v2, I think. Yes, that's fixed this time... > >> And we certainly want to keep the group information separate from >> the normal "runq" command. Here on a "live" 4 cpu Fedora 3.6.3 kernel, >> the command output exceeds 1000 lines! I'm pretty sure that most >> people will *not* want to see all of this: >> >> crash> runq -g >> CPU 0 >> CURRENT: PID: 0 TASK: ffffffff81c13420 COMMAND: "swapper/0" >> RT PRIO_ARRAY: ffff88021e213e28 >> [100] GROUP RT PRIO_ARRAY: ffff88020db22000 <system> >> [100] GROUP RT PRIO_ARRAY: ffff8801f8180000 <udisks2.service> >> [no tasks queued] > > <snip> > >> [no tasks queued] >> GROUP CFS RB_ROOT: ffff88020a844128 >> [no tasks queued] >> crash> >> >> In fact, it's difficult to actually find a real *task* that is on >> a run queue from among all of the empty "[no tasks queued]" groups! >> > > I've noticed this in the first patch and patch V2 has fixed this. OK, although I didn't realize that was a bug? What were all of those empty groups that are no longer shown? Are they actually *not* queued?

> > So I attach the new patch v2 version for runq -g. If you still find > any bug in your tests or have any suggestion about it, that's very > helpful. > > TODO: > 1. The help info about the -g option. > 2. Change rt_rq tasks displayed non-hierarchically. Like I mentioned above, the latest patch does not change the default behavior of runq alone, and "runq -g" is not as verbose as the last patch, which I presume is your intent.