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 <crash-utility-request@xxxxxxxxxx> wrote:
Date: Fri, 21 Jan 2022 13:43:09 +0800
From: Tao Liu <ltao@xxxxxxxxxx>
To: crash-utility@xxxxxxxxxx
Subject: [Crash-utility] [PATCH] Improve the ps performance for
vmcores with large number of threads
Message-ID: <20220121054309.9782-1-ltao@xxxxxxxxxx>
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 <ltao@xxxxxxxxxx>
---
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(?????) <k-hagio-ab@xxxxxxx>
To: lijiang <lijiang@xxxxxxxxxx>
Cc: "crash-utility@xxxxxxxxxx" <crash-utility@xxxxxxxxxx>
Subject: Re: [Crash-utility] [PATCH] Remove ptype command from "ps -t"
option to reduce memory and time
Message-ID:
<TYYPR01MB6777EC6D0A3A1847AA853965DD5B9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Content-Type: text/plain; charset="utf-8"
-----Original Message-----
> On Wed, Jan 19, 2022 at 4:27 PM HAGIO KAZUHITO(?????) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>
> > 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 <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx> >
> ---
> 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 <lijiang@xxxxxxxxxx <mailto:lijiang@xxxxxxxxxx> >
>
Thank you for reviewing, applied.
Kazu
------------------------------
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
End of Crash-utility Digest, Vol 196, Issue 15
**********************************************
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility
