Hi Bernhard, I'm sending new patch and the result of bloatcheck. For me, this size is reduced to 135 bytes. What do you think? Can you take a look at these attachments?
PS: The function of count_digits() is implemented inside of display_process_list(). To reduce the size, strlen() is not used. Br-Matthew ________________________________ From: Bernhard Reutner-Fischer <rep.dot....@gmail.com> Sent: Thursday, February 15, 2024 9:46 PM To: Matthew Chae <matthew.c...@axis.com> Cc: rep.dot....@gmail.com <rep.dot....@gmail.com>; David Laight <david.lai...@aculab.com>; 'Denys Vlasenko' <vda.li...@googlemail.com>; busybox@busybox.net <busybox@busybox.net>; Christopher Wong <christopher.w...@axis.com> Subject: Re: fix large PID overflow their column in top command On Wed, 14 Feb 2024 14:05:15 +0000 Matthew Chae <matthew.c...@axis.com> wrote: > Hi Bernhard, > > I'm sending new patch and the result of bloatcheck. Many thanks for the updated patch! function old new delta display_process_list 1406 1765 +359 .rodata 99721 99724 +3 ------------------------------------------------------------------------------ (add/remove: 0/0 grow/shrink: 2/0 up/down: 362/0) Total: 362 bytes text data bss dec hex filename 1009548 16507 1840 1027895 faf37 busybox_old 1009910 16507 1840 1028257 fb0a1 busybox_unstripped I think that's too much. For me this gives +293 bytes, still way too much. Can you please see if it helps to retain the manual formatting of PID PPID USER? PS: procps/top.c: In function ‘display_process_list’: procps/top.c:664:1: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] 664 | typedef struct { unsigned quot, rem; } bb_div_t; | ^~~~~~~ so please move your new #define PID_STR block down to right before /* what info of the processes is shown */ In + int lines = (lines_rem - 1); please drop the superfluous braces. It is most likely not smaller to use pid_len = strlen(make_human_readable_str(pid_max,0,0)) than to introduce this private count_digits(), i fear? Maybe we could amortize the addition of count_digits by reusing it elsewhere, as a follow-up. thanks > Can you review these and give me your thoughts? > Just let me know if you think that the code size needs to be reduced. > > Br-Matthew > ________________________________ > From: Bernhard Reutner-Fischer <rep.dot....@gmail.com> > Sent: Tuesday, February 13, 2024 7:16 PM > To: Matthew Chae <matthew.c...@axis.com> > Cc: rep.dot....@gmail.com <rep.dot....@gmail.com>; David Laight > <david.lai...@aculab.com>; 'Denys Vlasenko' <vda.li...@googlemail.com>; > busybox@busybox.net <busybox@busybox.net>; Christopher Wong > <christopher.w...@axis.com> > Subject: Re: fix large PID overflow their column in top command > > On Mon, 5 Feb 2024 09:56:20 +0000 > Matthew Chae <matthew.c...@axis.com> wrote: > > > Hi David, > > > > I'm sending an improved patch based on your comments. > > > > Not only does it not care about the PID_MAX value, > > it searches all the contents to be output to recognize the required column > > width > > and dynamically allocates the space for PID and PPID appropriately without > > creating a lot of empty space. > > > > Additionally, this patch still allows user names to be displayed up to 8 > > characters without truncation. > > > > Can you look through the attachment? > > Unfortunately the patch does not apply to current master. > How much would your patch add to the size? Can you bring it down to a > minimum? > See make baseline; apply the patch;make bloatcheck > > thanks > > > (0002-Allocate-PID-PPID-space-dynamically-in-top-command.patch) > > > > BR-Matthew Chae > > ________________________________ > > From: David Laight <david.lai...@aculab.com> > > Sent: Thursday, November 23, 2023 6:10 PM > > To: 'Denys Vlasenko' <vda.li...@googlemail.com>; Matthew Chae > > <matthew.c...@axis.com> > > Cc: busybox@busybox.net <busybox@busybox.net>; Christopher Wong > > <christopher.w...@axis.com> > > Subject: RE: fix large PID overflow their column in top command > > > > ... > > > + fp = xfopen_for_read("/proc/sys/kernel/pid_max"); > > > + if (!fgets(pid_buf, PID_DIGITS_MAX + 1, fp)) { > > > ... > > > + if (strncmp(pid_buf, "32768", 5) == 0) > > > + pid_digits_num = 5; > > > + else > > > + pid_digits_num = PID_DIGITS_MAX; > > > > > > The logic above is not sound. Even if sysctl kernel.pid_max > > > is 32768, there can be already running processes with pids > 99999. > > > > It's also probably wrong for pretty much all other values. > > > > I'd just base the column width on strlen(pid_buf) with a minimum > > value of 5. > > > > It is unlikely that pid_max has been reduced - so column overflow > > it that case probably doesn't really matter. > > > > The more interesting case is really a system with a very large pid_max > > that has never run many processes. > > You don't really want lots of blank space. > > > > I can't remember whether top reads everything before doing any output? > > Since the output is sorted it probably almost always does. > > In which case it knows the column width it will need. > > > > I did post a patch a while back that enabled 'Irix mode'. > > (100% cpu is one cpu at 100%, not all cpus at 100%) > > Maybe I should dig it out again. > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > > 1PT, UK > > Registration No: 1397386 (Wales) >
bloatcheck_top_0004
Description: bloatcheck_top_0004
From c9b63b5c82c9f44045c9fd8febf0e1c776becb47 Mon Sep 17 00:00:00 2001 From: Matthew Chae <matth...@axis.com> Date: Mon, 19 Feb 2024 11:12:31 +0100 Subject: [PATCH] Allocate PID/PPID space dynamically in top command The presence of a large number of PID and PPID digits in the column may cause overflow, making it impossible to display the entire data accurately. This dynamically allocates the appropriate space for the number of digits in the PID and PPID, ensuring the correct representation of values in each column and guaranteeing a clean display. Reviewed-by: Christopher Wong <christopher.w...@axis.com> Signed-off-by: Matthew(Sukyoung) Chae <matth...@axis.com> --- procps/top.c | 69 +++++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/procps/top.c b/procps/top.c index 09d31c673..47eaa8a27 100644 --- a/procps/top.c +++ b/procps/top.c @@ -637,9 +637,41 @@ typedef struct { unsigned quot, rem; } bb_div_t; # define FMT "%4u%%" #endif + int lines = lines_rem - 1; + unsigned pid_max = 0; + unsigned ppid_max = 0; + unsigned pid_digits_max = 0; + unsigned ppid_digits_max = 0; + top_status_t *s_tmp = top + G_scroll_ofs; + + if (lines > ntop - G_scroll_ofs) + lines = ntop - G_scroll_ofs; + + while (--lines >= 0) { + pid_max = MAX(pid_max, s_tmp->pid); + ppid_max = MAX(ppid_max, s_tmp->ppid); + s_tmp++; + } + + while (pid_max > 0) { + pid_max /= 10; + pid_digits_max++; + } + pid_digits_max = MAX(5, pid_digits_max); /* The minimum digits for PID is 5. */ + + while (ppid_max > 0) { + ppid_max /= 10; + ppid_digits_max++; + } + ppid_digits_max = MAX(5, ppid_digits_max); /* The minimum digits for PPID is 5. */ + /* what info of the processes is shown */ + printf(OPT_BATCH_MODE ? "%*s%s %*s%s " : ESC"[7m" "%*s%s %*s%s " ESC"[m", + pid_digits_max - 3, " ", "PID", /* The length of the PID string : 3 */ + ppid_digits_max - 4, " ", "PPID"); /* The length of the PPID string : 4 */ + printf(OPT_BATCH_MODE ? "%.*s" : ESC"[7m" "%.*s" ESC"[m", scr_width, - " PID PPID USER STAT VSZ %VSZ" + "USER STAT VSZ %VSZ" IF_FEATURE_TOP_SMP_PROCESS(" CPU") IF_FEATURE_TOP_CPU_USAGE_PERCENTAGE(" %CPU") " COMMAND"); @@ -696,9 +728,6 @@ typedef struct { unsigned quot, rem; } bb_div_t; lines_rem = ntop - G_scroll_ofs; s = top + G_scroll_ofs; while (--lines_rem >= 0) { - int n; - char *ppu; - char ppubuf[sizeof(int)*3 * 2 + 12]; char vsz_str_buf[8]; unsigned col; @@ -709,39 +738,13 @@ typedef struct { unsigned quot, rem; } bb_div_t; smart_ulltoa5(s->vsz, vsz_str_buf, " mgtpezy"); /* PID PPID USER STAT VSZ %VSZ [%CPU] COMMAND */ - n = sprintf(ppubuf, "%5u %5u %-8.8s", s->pid, s->ppid, get_cached_username(s->uid)); - ppu = ppubuf; - if (n != 6+6+8) { - /* Format PID PPID USER part into 6+6+8 chars: - * shrink PID/PPID if possible, then truncate USER. - * Tested on Linux 5.18.0: - * sysctl kernel.pid_max=4194304 is the maximum allowed, - * so PID and PPID are 7 chars wide at most. - */ - char *p, *pp; - if (*ppu == ' ') { - do { - ppu++, n--; - if (n == 6+6+8) - goto shortened; - } while (*ppu == ' '); - } - pp = p = skip_non_whitespace(ppu) + 1; - if (*p == ' ') { - do - p++, n--; - while (n != 6+6+8 && *p == ' '); - overlapping_strcpy(pp, p); /* shrink PPID */ - } - ppu[6+6+8] = '\0'; /* truncate USER */ - } - shortened: col = snprintf(line_buf, scr_width, - "\n" "%s %s %.5s" FMT + "\n" "%*u %*u %-8.8s %s %.5s" FMT IF_FEATURE_TOP_SMP_PROCESS(" %3d") IF_FEATURE_TOP_CPU_USAGE_PERCENTAGE(FMT) " ", - ppu, + pid_digits_max, s->pid, ppid_digits_max, s->ppid, + get_cached_username(s->uid), s->state, vsz_str_buf, SHOW_STAT(pmem) IF_FEATURE_TOP_SMP_PROCESS(, s->last_seen_on_cpu) -- 2.30.2
_______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox