Re: [Crash-utility] Speed up list/tree '-s' output.

----- Original Message ----- 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