[Crash-utility] [PATCH] Add the proccgroup extension

[Dave Anderson]

----- Original Message -----
> Initial version of a crash module which can be used to show which cgroups
> is the currently active process member of.
> ---
> Hello this is a simple crash extension that I hacked up over the weekend, in my
> case when I look at kernel crash dump I want to quickly understand which cgroup
> is the current process member of. Currently it uses the process from the current
> context but this might change in the future. Here is an example output:
> 
> crash> show_cgroups
> subsys: cpuset               cgroup: c6666
> subsys: cpu                  cgroup: c6666
> subsys: cpuacct              cgroup: c6666
> subsys: io                   cgroup: c6666
> subsys: memory               cgroup: c6666
> subsys: devices              cgroup: c6666
> subsys: freezer              cgroup: c6666
> subsys: perf_event           cgroup: c6666
> subsys: pids                 cgroup: c6666
> 
> I have tested this on 4.6-rc2 with and without cgroup support enabled.
> 
> I'm just sending this to get an initial idea whether I have used crash's
> facilities correctly and canvas for future ideas. I'm aware there are already 2
> cgroup modules but when I tried running either they complained of no cgroup 
> support or the command did nothing. In any case provided that the code is ok I
> guess this can be used as a good example of how to traverse structures with crash
> 
> TODO:
>  * Make the command understand either task_struct pointer or pid being passed.
>  * Add support for pre-3.15 kernels (the cgroup name struct changed to kernfs at that point)
>  * Whatever people think might be useful


Hello Nikolay,

I have a few comments and suggestions:

+#include "defs.h"
+
+#define CGROUP_PATH_MAX

What is the purpose of CGROUP_PATH_MAX?  

+void proccgroup_init(void);
+void proccgroup_fini(void);
+
... [ cut ] ...
+
+void __attribute__((constructor))
+echo_init(void)
+{
+        register_extension(command_table);
+}
+
+void __attribute__((destructor))
+echo_fini(void) { }
+

You've got forward declarations of proccgroup_init() and proccgroup_fini(), 
but the two functions don't exist.  The "echo_xxx()" functions in the 
echo.c sample module are meant to be replaced by names reflecting your
module. 

If I run this module on a 3.10 kernel, I get this:
  
crash> extend proccgroup.so
./extensions/proccgroup.so: shared object loaded
crash> show_cgroups
Unsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionUnsupported kernel versionshow_cgroups: invalid kernel virtual address: 0  type: "cgroup_subsys_state->cgroup"
  Unsupported kernel version
crash>

because of this:
  
+        /* Handle the 2 cases of cgroup_name and the kernfs one */
+        if (MEMBER_EXISTS("cgroup", "kn")) {
+            get_kn_cgroup_name(cgroup, subsys_ptr);
+        } else if (MEMBER_EXISTS("cgroup", "name")) {
+            fprintf(fp, "Unsupported kernel version");
+        }

You should replace the "fprintf(fp, "Unsupported kernel version")" error message
above with an "error(FATAL, ...)" construct (and add a "\n" to the end), so that
the command will fail immediately and return to the "crash> " prompt.

Alternatively, you can have your module purposely fail to load if any required
structure members do not exist.  Typically modules will have their xxxx_init() 
constructor function do some pre-checks of the kernel being analyzed, and if the
command will not be able to run correctly, return before calling register_extension():
  
  void __attribute__((constructor))
  xxxx_init(void)
  {
   	if (!MEMBER_EXISTS(...) || !MEMBER_EXISTS(...)) {
  		fprintf(fp, "some error message\n");
  		return;
  	}
  
	register_extension(command_table);
  }

I note that all of your readmem() calls use RETURN_ON_ERROR, such as these examples:

+static void get_cgroup_name(ulong cgroup, char *buf, size_t buflen) 
+{
+
+    ulong kernfs_node;
+    ulong cgroup_name_ptr;
+    ulong kernfs_parent;
+
+    /* Get cgroup->kn */
+    readmem(cgroup + MEMBER_OFFSET("cgroup", "kn"), KVADDR, &kernfs_node, sizeof(void *),
+            "cgroup->kn", RETURN_ON_ERROR);
+
+    readmem(kernfs_node + MEMBER_OFFSET("kernfs_node", "parent"), KVADDR, &kernfs_parent, sizeof(void *),
+            "kernfs_node->parent", RETURN_ON_ERROR);
+
+    if (kernfs_parent == 0) {
+        sprintf(buf, "/");
+        return;
+    }
+
+    /* Get kn->name */
+    readmem(kernfs_node + MEMBER_OFFSET("kernfs_node", "name"), KVADDR, &cgroup_name_ptr, sizeof(void *),
+            "kernfs_node->name", RETURN_ON_ERROR);
+
+    read_string(cgroup_name_ptr, buf, buflen-1);
+}

Unless you can do something useful in the case of a readmem() error, it's best to
use FAULT_ON_ERROR.  You will get an error message automatically generated by readmem(),
but similar to "error(FATAL, ...)" it will return immediately to the "crash> " prompt.
In your module above, you would just get a stream of confusing error messages.
  
You might consider changing the name of the command to something that
flows off the fingertips a bit easier:

+static struct command_table_entry command_table[] = {
+        { "show_cgroups", show_proc_cgroups, help_proc_cgroups, 0},
+        { NULL },
+};

Maybe something like "cgrp", or "showcg", or anything short but to
the point.
 
Thanks,
  Dave