[Crash-utility] Speed up list/tree '-s' output.
Dave Anderson
anderson at redhat.com
Tue Aug 30 15:54:22 UTC 2016
----- Original Message -----
>
>
> ----- Original Message -----
> > Hi Dave,
> >
> ... [ cut ] ...
>
> > The patch for crash tool can be found on github:
> > https://raw.githubusercontent.com/hziSot/crash-patches/41abc6f9c91c2a602973ff9564234395deadb80e/crash_speed_up_tree_list.patch
> >
> > Thanks,
> > Alexandr
> >
>
Hi Alexandr,
Now, aside from the functionality issues in my previous email, let's move on to the implemention...
I'd like to clarify and simplify the patch a bit -- my comments are interleaved below.
diff --git a/defs.h b/defs.h
index d6691a3..c2c76c2 100644
--- a/defs.h
+++ b/defs.h
@@ -2380,6 +2380,12 @@ struct datatype_member { /* minimal definition of a structure/union */
};
#define union_name struct_name
+struct req_entry {
+ char *arg, *name;
+ char **member, *width, *is_ptr;
+ short *offset;
+ unsigned char count;
+};
This req_entry data structure is confusing as hell, primarily
because things like the is_ptr, width and offset fields seem to
be mis-typed. I understand that they are pointers to arrays,
but why not make them pointers to their real types instead of
"char *" and "short *"? By that I mean is_ptr is a long returned
by MEMBER_TYPE(), width is a long returned by ANON_MEMBER_OFFSET(),
and offset is a long returned by MEMBER_OFFSET(). I presume you
made them "char *" because they use GETBUF(), but typically the
users of GETBUF() will cast the return value from GETBUF() as a
pointer to what it really points to. Now, I also understand that
you may feel that it would be a waste of allocated space, but
keep in mind that GETBUF() parses out the pre-allocated buffers
first, so while you may think you're getting just a few bytes,
you're really getting 1K or 2K buffers. So it really makes more
sense to type-cast them as what they really are.
Now, with respect to how the req_entry structure is appended to the
list_data and tree_data structures:
struct list_data { /* generic structure used by do_list() to walk */
ulong flags; /* through linked lists in the kernel */
@@ -2395,6 +2401,8 @@ struct list_data { /* generic structure used by do_list() to walk */
int (*callback_func)(void *, void *);
void *callback_data;
long struct_list_offset;
+ int entries;
+ struct req_entry **e;
};
#define LIST_OFFSET_ENTERED (VERBOSE << 1)
#define LIST_START_ENTERED (VERBOSE << 2)
@@ -2416,6 +2424,8 @@ struct tree_data {
char **structname;
int structname_args;
int count;
+ int entries;
+ struct req_entry **e;
};
The new "entries" and "e" pointers are really just duplications of the
existing "structname", "structname_args" and "count" members. In the
do_list() and do_tree() functions, there's no reason you couldn't
continue to use those existing fields.
Furthermore, I see no reason that you have to export the req_entry
structure in defs.h. It's really an implementation-only structure
that is used internally in tools.c. Please declare it there only.
diff --git a/tools.c b/tools.c
index 2c3aab9..1b5420e 100644
--- a/tools.c
+++ b/tools.c
@@ -27,6 +27,9 @@ static void dump_struct_members(struct list_data *, int, ulong);
static void rbtree_iteration(ulong, struct tree_data *, char *);
static void rdtree_iteration(ulong, struct tree_data *, char *, ulong, uint);
static void dump_struct_members_for_tree(struct tree_data *, int, ulong);
+static void print_value(char *, ulong, short, unsigned int);
+static struct req_entry *fill_member_offsets(char *);
+static void dump_struct_members_fast(struct req_entry *, int, ulong);
/*
* General purpose error reporting routine. Type INFO prints the message
@@ -3222,6 +3225,7 @@ cmd_list(void)
struct list_data list_data, *ld;
struct datatype_member struct_member, *sm;
struct syment *sp;
+ struct req_entry *re;
ulong value, struct_list_offset;
sm = &struct_member;
@@ -3229,7 +3233,7 @@ cmd_list(void)
BZERO(ld, sizeof(struct list_data));
struct_list_offset = 0;
- while ((c = getopt(argcnt, args, "Hhrs:e:o:xdl:")) != EOF) {
+ while ((c = getopt(argcnt, args, "Hhrs:S:e:o:xdl:")) != EOF) {
switch(c)
{
case 'H':
@@ -3246,11 +3250,18 @@ cmd_list(void)
break;
case 's':
- if (ld->structname_args++ == 0)
+ if (ld->structname_args++ == 0)
hq_open();
hq_enter((ulong)optarg);
break;
+ case 'S':
+ re = fill_member_offsets(optarg);
+ if (ld->entries++ == 0)
+ hq_open();
+ hq_enter((ulong)re);
+ break;
+
The 's' and 'S' case handlers above could be almost identical. Instead
of stashing the "re" pointers, save the optarg variables. For mutual exclusion,
do what the other crash commands do, and create local "sflag" and "Sflag" variables.
To differentiate the -s and -S option later when they are acted upon, create another
LIST_XXXX flag and set it above in ld->flags. The req_entry allocation(s) and
fill_member_offsets() call(s) can be done later.
case 'l':
if (IS_A_NUMBER(optarg))
struct_list_offset = stol(optarg,
@@ -3313,13 +3324,24 @@ cmd_list(void)
cmd_usage(pc->curcmd, SYNOPSIS);
}
+ if (ld->structname_args && ld->entries) {
+ error(INFO, "-S and -s options are mutually exclusive");
+ cmd_usage(pc->curcmd, SYNOPSIS);
+ }
+
if (ld->structname_args) {
ld->structname = (char **)GETBUF(sizeof(char *) * ld->structname_args);
retrieve_list((ulong *)ld->structname, ld->structname_args);
hq_close();
ld->struct_list_offset = struct_list_offset;
+ } else if (ld->entries) {
+ ld->e = (struct req_entry **)GETBUF(sizeof (struct req_entry *) *
+ ld->entries);
+ retrieve_list((ulong *)ld->e, ld->entries);
+ hq_close();
+ ld->struct_list_offset = struct_list_offset;
Again, the "if (ld->entries)" section above could be removed entirely.
} else if (struct_list_offset) {
- error(INFO, "-l option can only be used with -s option\n");
+ error(INFO, "-l option can only be used with -s or -S option\n");
cmd_usage(pc->curcmd, SYNOPSIS);
}
@@ -3483,6 +3505,102 @@ next_arg:
FREEBUF(ld->structname);
}
+void
+dump_struct_members_fast(struct req_entry *e, int radix, ulong p)
+{
+ unsigned int i;
+ char b[BUFSIZE];
+ if (!IS_KVADDR(p))
+ return;
+ for (i = 0; i < e->count; i++) {
+ if (e->width[i] == 0 || e->width[i] > 8) {
+ snprintf(b, BUFSIZE, "%s.%s", e->name, e->member[i]);
+ dump_struct_member(b, p, radix);
+ } else {
+ print_value(e->member[i], p + e->offset[i], e->width[i],
+ e->is_ptr[i] ? 16 : radix);
+ }
+ }
+}
+
+static struct req_entry *
+fill_member_offsets(char *arg)
+{
+ int j;
+ char *p, m;
+ struct req_entry *e;
+ char b[BUFSIZE];
+
+ if (!(arg && *arg))
+ return NULL;
+
+ j = count_chars(arg, ',') + 1;
+ e = (struct req_entry *)GETBUF(sizeof(*e));
+
+ e->arg = GETBUF(strlen(arg + 1));
+ strcpy(e->arg, arg);
+
+ m = ((p = strchr(e->arg, '.')) != NULL);
+ if (!p++)
+ p = e->arg + strlen(e->arg) + 1;
+
+ e->name = GETBUF(p - e->arg);
+ strncpy(e->name, e->arg, p - e->arg - 1);
+
+ if (!m)
+ return e;
+
+ e->count = count_chars(p, ',') + 1;
+ e->width = GETBUF(e->count);
+ e->is_ptr = GETBUF(e->count);
+ e->member = (char **)GETBUF(e->count * sizeof(char *));
+ e->offset = (short *)GETBUF(e->count * sizeof(short));
+
+ replace_string(p, ",", ' ');
+ parse_line(p, e->member);
+
+ for (j = 0; j < e->count; j++) {
+ e->offset[j] = MEMBER_OFFSET(e->name, e->member[j]);
+ if (e->offset[j] == -1)
+ e->offset[j] = ANON_MEMBER_OFFSET(e->name, e->member[j]);
+ if (e->offset[j] == -1)
+ break;
+
+ e->is_ptr[j] = MEMBER_TYPE(e->name, e->member[j]) == TYPE_CODE_PTR;
+ // Dirty hack for obtaining size of particular field
+ snprintf(b, BUFSIZE, "%s + 1", e->member[j]);
+ e->width[j] = ANON_MEMBER_OFFSET(e->name, b) - e->offset[j];
+ }
+
+ return e;
+}
+
+static void
+print_value(char *name, ulong addr, short width, unsigned int radix)
+{
+ union { uint64_t v64; uint32_t v32;
+ uint16_t v16; uint8_t v8;
+ } v;
+ char fmt[BUFSIZE];
+
+ if (!readmem(addr, KVADDR, &v, width,
+ "structure value", RETURN_ON_ERROR | QUIET)) {
+ error(INFO, "cannot access field: %s at %lx\n", name, addr);
+ return;
+ }
+ snprintf(fmt, BUFSIZE, " %%s = %s%%%s%s\n",
+ (radix == 16 ? "0x" : ""),
+ (width == 8 ? "l" : ""),
+ (radix == 16 ? "x" : "u" )
+ );
+
+ switch (width) {
+ case 1: fprintf(fp, fmt, name, v.v8); break;
+ case 2: fprintf(fp, fmt, name, v.v16); break;
+ case 4: fprintf(fp, fmt, name, v.v32); break;
+ case 8: fprintf(fp, fmt, name, v.v64); break;
+ }
+}
/*
* Does the work for cmd_list() and any other function that requires the
@@ -3491,10 +3609,11 @@ next_arg:
int
do_list(struct list_data *ld)
{
- ulong next, last, first;
+ ulong next, last, first, offset;
ulong searchfor, readflag;
- int i, count, others, close_hq_on_return;
+ int i, j, count, others, close_hq_on_return;
unsigned int radix;
+ char b[BUFSIZE];
if (CRASHDEBUG(1)) {
others = 0;
@@ -3536,6 +3655,16 @@ do_list(struct list_data *ld)
console(" structname: (unused)\n");
for (i = 0; i < ld->structname_args; i++)
console(" structname[%d]: %s\n", i, ld->structname[i]);
+ console(" entries: %lx\n", ld->entries);
+ for (i = 0; i < ld->entries; i++) {
+ console(" entry[%d]: ", i);
+ for (j = 0; j < ld->e[i]->count; j++) {
+ snprintf(b, BUFSIZE, "{ member: %s, width: %d, offset: %d }",
+ ld->e[i]->member[j], ld->e[i]->width[j], ld->e[i]->offset[j]);
+ console("%s", b);
+ console(j == ld->e[i]->count - 1 ? "\n" : ", ");
+ }
+ }
console(" header: %s\n", ld->header);
console(" list_ptr: %lx\n", (ulong)ld->list_ptr);
console(" callback_func: %lx\n", (ulong)ld->callback_func);
@@ -3580,6 +3709,7 @@ do_list(struct list_data *ld)
if (ld->header)
fprintf(fp, "%s", ld->header);
+ offset = ld->list_head_offset + ld->struct_list_offset;
while (1) {
if (ld->flags & VERBOSE) {
fprintf(fp, "%lx\n", next - ld->list_head_offset);
@@ -3589,9 +3719,8 @@ do_list(struct list_data *ld)
switch (count_chars(ld->structname[i], '.'))
{
case 0:
- dump_struct(ld->structname[i],
- next - ld->list_head_offset - ld->struct_list_offset,
- radix);
+ dump_struct(ld->structname[i],
+ next - offset, radix);
break;
default:
dump_struct_members(ld, i, next);
@@ -3599,6 +3728,8 @@ do_list(struct list_data *ld)
}
}
}
+ for (i = 0; i < ld->entries; i++)
+ dump_struct_members_fast(ld->e[i], radix, next - offset);
}
Move the section above into the "if (ld->structname)" section, and
check the new LIST_XXX flag. The req_entry allocation(s) and the calls
to fill_member_offsets() should be able to be done somewhere around
here.
if (next && !hq_enter(next - ld->list_head_offset)) {
@@ -3737,6 +3868,7 @@ cmd_tree()
struct tree_data tree_data, *td;
struct datatype_member struct_member, *sm;
struct syment *sp;
+ struct req_entry *re;
ulong value;
type_flag = 0;
@@ -3745,7 +3877,7 @@ cmd_tree()
td = &tree_data;
BZERO(td, sizeof(struct tree_data));
- while ((c = getopt(argcnt, args, "xdt:r:o:s:pN")) != EOF) {
+ while ((c = getopt(argcnt, args, "xdt:r:o:s:S:pN")) != EOF) {
switch (c)
{
case 't':
@@ -3804,6 +3936,13 @@ cmd_tree()
hq_enter((ulong)optarg);
break;
+ case 'S':
+ re = fill_member_offsets(optarg);
+ if (td->entries++ == 0)
+ hq_open();
+ hq_enter((ulong)re);
+ break;
+
Same comments as cmd_list()...
case 'p':
td->flags |= TREE_POSITION_DISPLAY;
break;
@@ -3878,11 +4017,21 @@ next_arg:
cmd_usage(pc->curcmd, SYNOPSIS);
}
+ if (td->structname_args && td->entries) {
+ error(INFO, "-S and -s options are mutually exclusive");
+ cmd_usage(pc->curcmd, SYNOPSIS);
+ }
+
if (td->structname_args) {
td->structname = (char **)GETBUF(sizeof(char *) *
td->structname_args);
retrieve_list((ulong *)td->structname, td->structname_args);
hq_close();
+ } else if (td->entries) {
+ td->e = (struct req_entry **)GETBUF(sizeof (struct req_entry *) *
+ td->entries);
+ retrieve_list((ulong *)td->e, td->entries);
+ hq_close();
}
if (!(td->flags & TREE_NODE_POINTER))
@@ -3917,6 +4066,7 @@ next_arg:
fprintf(fp, " start: %lx\n", td->start);
fprintf(fp, "node_member_offset: %ld\n", td->node_member_offset);
fprintf(fp, " structname_args: %d\n", td->structname_args);
+ fprintf(fp, " entries: %d\n", td->entries);
fprintf(fp, " count: %d\n", td->count);
}
@@ -4031,6 +4181,13 @@ rdtree_iteration(ulong node_p, struct tree_data *td, char *ppos, ulong indexnum,
else
sprintf(pos, "%s", ppos);
+ if (td->flags & TREE_STRUCT_RADIX_10)
+ print_radix = 10;
+ else if (td->flags & TREE_STRUCT_RADIX_16)
+ print_radix = 16;
+ else
+ print_radix = 0;
+
for (index = 0; index < RADIX_TREE_MAP_SIZE; index++) {
readmem((ulong)node_p + OFFSET(radix_tree_node_slots) +
sizeof(void *) * index, KVADDR, &slot, sizeof(void *),
@@ -4052,13 +4209,6 @@ rdtree_iteration(ulong node_p, struct tree_data *td, char *ppos, ulong indexnum,
fprintf(fp, " position: %s/%d\n", pos, index);
if (td->structname) {
- if (td->flags & TREE_STRUCT_RADIX_10)
- print_radix = 10;
- else if (td->flags & TREE_STRUCT_RADIX_16)
- print_radix = 16;
- else
- print_radix = 0;
-
for (i = 0; i < td->structname_args; i++) {
switch(count_chars(td->structname[i], '.'))
{
@@ -4073,6 +4223,8 @@ rdtree_iteration(ulong node_p, struct tree_data *td, char *ppos, ulong indexnum,
}
}
}
+ for (i = 0; i < td->entries; i++)
+ dump_struct_members_fast(td->e[i], print_radix, slot);
Same as do_list() -- move it up into the "if (td->structname)" section,
check for the new LIST_XXX pointer, and do the pre-allocations around
here.
} else
rdtree_iteration(slot, td, pos, index, height-1);
}
Here's something I didn't notice until now. You set things up
in cmd_tree() for subsequent execution in rdtree_iteration().
But what about rbtree_iteration()? Did you skip it on purpose?
Thanks,
Dave
More information about the Crash-utility
mailing list