Thank you for the improvement, Tao. This change looks good to me. Applied(but modified code comments).
Thanks. Lianbo On Fri, Jan 21, 2022 at 3:40 PM <[email protected]> wrote: > > Date: Fri, 21 Jan 2022 13:43:09 +0800 > From: Tao Liu <[email protected]> > To: [email protected] > Subject: [Crash-utility] [PATCH] Improve the ps performance for > vmcores with large number of threads > Message-ID: <[email protected]> > Content-Type: text/plain; charset="US-ASCII" > > Previously, the ps command will iterate over all threads which > have the same tgid, to accumulate their rss value, in order to > get a thread/process's final rss value as part of the final output. > > For non-live systems, the rss accumulation values are identical for > threads which have the same tgid, so there is no need to do the > iteration and accumulation repeatly, thus a lot of readmem calls are > skipped. Otherwise it will be the performance bottleneck if the > vmcores have a large number of threads. > > In this patch, the rss accumulation value will be stored in a cache, > next time a thread with the same tgid will take it directly without > the iteration. > > For example, we can monitor the performance issue when a vmcore has > ~65k processes, most of which are threads for several specific > processes. Without the patch, it will take ~7h for ps command > to finish. With the patch, ps command will finish in 1min. > > Signed-off-by: Tao Liu <[email protected]> > --- > defs.h | 1 + > memory.c | 67 ++++++++++++++++++++++++++++++-------------------------- > task.c | 1 + > 3 files changed, 38 insertions(+), 31 deletions(-) > > diff --git a/defs.h b/defs.h > index b63741c..55600d5 100644 > --- a/defs.h > +++ b/defs.h > @@ -829,6 +829,7 @@ struct task_context { /* context > stored for each task */ > struct tgid_context { /* tgid and task stored for each task > */ > ulong tgid; > ulong task; > + long rss_cache; > }; > > struct task_table { /* kernel/local task table data > */ > diff --git a/memory.c b/memory.c > index 5af45fd..b930933 100644 > --- a/memory.c > +++ b/memory.c > @@ -4665,7 +4665,7 @@ void > get_task_mem_usage(ulong task, struct task_mem_usage *tm) > { > struct task_context *tc; > - long rss = 0; > + long rss = 0, rss_cache = 0; > > BZERO(tm, sizeof(struct task_mem_usage)); > > @@ -4730,38 +4730,43 @@ get_task_mem_usage(ulong task, struct > task_mem_usage *tm) > (last->tgid == (last + 1)->tgid)) > last++; > > - while (first <= last) > - { > - /* count 0 -> filepages */ > - if (!readmem(first->task + > - > OFFSET(task_struct_rss_stat) + > - > OFFSET(task_rss_stat_count), KVADDR, > - &sync_rss, > - sizeof(int), > - "task_struct rss_stat > MM_FILEPAGES", > - RETURN_ON_ERROR)) > - continue; > - > - rss += sync_rss; > - > - /* count 1 -> anonpages */ > - if (!readmem(first->task + > - > OFFSET(task_struct_rss_stat) + > - > OFFSET(task_rss_stat_count) + > - sizeof(int), > - KVADDR, &sync_rss, > - sizeof(int), > - "task_struct rss_stat > MM_ANONPAGES", > - RETURN_ON_ERROR)) > - continue; > - > - rss += sync_rss; > - > - if (first == last) > - break; > - first++; > + /* We will use rss_cache only for dumpfile > */ > + if (ACTIVE() || last->rss_cache == > UNINITIALIZED) { > + while (first <= last) > + { > + /* count 0 -> filepages */ > + if (!readmem(first->task + > + > OFFSET(task_struct_rss_stat) + > + > OFFSET(task_rss_stat_count), KVADDR, > + &sync_rss, > + sizeof(int), > + "task_struct > rss_stat MM_FILEPAGES", > + RETURN_ON_ERROR)) > + continue; > + > + rss_cache += sync_rss; > + > + /* count 1 -> anonpages */ > + if (!readmem(first->task + > + > OFFSET(task_struct_rss_stat) + > + > OFFSET(task_rss_stat_count) + > + sizeof(int), > + KVADDR, &sync_rss, > + sizeof(int), > + "task_struct > rss_stat MM_ANONPAGES", > + RETURN_ON_ERROR)) > + continue; > + > + rss_cache += sync_rss; > + > + if (first == last) > + break; > + first++; > + } > + last->rss_cache = rss_cache; > } > > + rss += last->rss_cache; > tt->last_tgid = last; > } > } > diff --git a/task.c b/task.c > index 263a834..9868a6e 100644 > --- a/task.c > +++ b/task.c > @@ -2947,6 +2947,7 @@ add_context(ulong task, char *tp) > tg = tt->tgid_array + tt->running_tasks; > tg->tgid = *tgid_addr; > tg->task = task; > + tg->rss_cache = UNINITIALIZED; > > if (do_verify && !verify_task(tc, do_verify)) { > error(INFO, "invalid task address: %lx\n", tc->task); > -- > 2.33.1 > > > > ------------------------------ > > Message: 3 > Date: Fri, 21 Jan 2022 07:35:22 +0000 > From: HAGIO KAZUHITO(?????) <[email protected]> > To: lijiang <[email protected]> > Cc: "[email protected]" <[email protected]> > Subject: Re: [Crash-utility] [PATCH] Remove ptype command from "ps -t" > option to reduce memory and time > Message-ID: > < > tyypr01mb6777ec6d0a3a1847aa853965dd...@tyypr01mb6777.jpnprd01.prod.outlook.com > > > > Content-Type: text/plain; charset="utf-8" > > -----Original Message----- > > On Wed, Jan 19, 2022 at 4:27 PM HAGIO KAZUHITO(?????) < > [email protected] <mailto:[email protected]> > > > wrote: > > > > > > With some vmlinux e.g. RHEL9 ones, the first execution of the gdb > ptype > > command heavily consumes memory and time. The "ps -t" option uses > it > > in start_time_timespec() and it can be replaced with crash macros. > > This can reduce about 1.4 GB memory and 6 seconds time comsumption > in > > the following test: > > > > $ echo "ps -t" | time crash vmlinux vmcore > > > > Without the patch: > > 11.60user 0.43system 0:11.94elapsed 100%CPU (0avgtext+0avgdata > 1837964maxresident)k > > 0inputs+400outputs (0major+413636minor)pagefaults 0swaps > > > > With the patch: > > 5.40user 0.16system 0:05.46elapsed 101%CPU (0avgtext+0avgdata > 417896maxresident)k > > 0inputs+384outputs (0major+41528minor)pagefaults 0swaps > > > > Although the ptype command and similar ones cannot be fully > removed, > > but removing some of them will make the use of crash safer, > especially > > for an automatic crash analyzer. > > > > Signed-off-by: Kazuhito Hagio <[email protected] <mailto: > [email protected]> > > > --- > > task.c | 25 +++++-------------------- > > 1 file changed, 5 insertions(+), 20 deletions(-) > > > > diff --git a/task.c b/task.c > > index 263a8344dd94..a79ed0d96fb5 100644 > > --- a/task.c > > +++ b/task.c > > @@ -4662,8 +4662,6 @@ show_task_times(struct task_context *tcp, > ulong flags) > > static int > > start_time_timespec(void) > > { > > - char buf[BUFSIZE]; > > - > > switch(tt->flags & (TIMESPEC | NO_TIMESPEC | > START_TIME_NSECS)) > > { > > case TIMESPEC: > > @@ -4677,24 +4675,11 @@ start_time_timespec(void) > > > > tt->flags |= NO_TIMESPEC; > > > > - open_tmpfile(); > > - sprintf(buf, "ptype struct task_struct"); > > - if (!gdb_pass_through(buf, NULL, GNU_RETURN_ON_ERROR)) { > > - close_tmpfile(); > > - return FALSE; > > - } > > - > > - rewind(pc->tmpfile); > > - while (fgets(buf, BUFSIZE, pc->tmpfile)) { > > - if (strstr(buf, "start_time;")) { > > - if (strstr(buf, "struct timespec")) { > > - tt->flags &= ~NO_TIMESPEC; > > - tt->flags |= TIMESPEC; > > - } > > - } > > - } > > - > > - close_tmpfile(); > > + if (VALID_MEMBER(task_struct_start_time) && > > + STREQ(MEMBER_TYPE_NAME("task_struct", "start_time"), > "timespec")) { > > + tt->flags &= ~NO_TIMESPEC; > > + tt->flags |= TIMESPEC; > > + } > > > > if ((tt->flags & NO_TIMESPEC) && > (SIZE(task_struct_start_time) == 8)) { > > tt->flags &= ~NO_TIMESPEC; > > -- > > 2.27.0 > > > > > > > > For this one: > > Acked-by: Lianbo Jiang <[email protected] <mailto:[email protected]> > > > > > Thank you for reviewing, applied. > > Kazu > > > > > ------------------------------ > > -- > Crash-utility mailing list > [email protected] > https://listman.redhat.com/mailman/listinfo/crash-utility > > End of Crash-utility Digest, Vol 196, Issue 15 > ********************************************** > >
-- Crash-utility mailing list [email protected] https://listman.redhat.com/mailman/listinfo/crash-utility
