Re: fix large PID overflow their column in top command
n the actual result. In reality, the
'Before' results of the 'top' command are much less organized than they
appear above.
PS: 135b is better than the initial suggestion of ~300b, but given
architectures tend to end up with very different codegen per arch and
compilers used, i'm always curious which arch and which compiler
(version) was used to obtain the alleged results. Guess you target
chris with gcc-12-ish?
Stating the target arch usually allows us a rough estimate
about overall impact on other arches.
I’m giving you the arch and compiler information below.
C Compiler: gcc -> gcc-10*
ARCH x86_64
Many thanks in advance and cheers,
Bernhard
Br-Matthew
From: Bernhard Reutner-Fischer
Sent: Thursday, February 15, 2024 9:46 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko'
; busybox@busybox.net
; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Wed, 14 Feb 2024 14:05:15 +
Matthew Chae 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
Sent: Tuesday, February 13, 2024 7:16 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko'
; busybox@busybox.net
; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Mon, 5 Feb 2024 09:56:20 +
Matthew Chae 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
Sent: Thursday, November 23, 2023 6:10 PM
To: 'Denys Vlasenko' ; Matthew Chae
Cc: busybox@busybox.net ; Christopher Wong
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 > 9.
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 p
Re: fix large PID overflow their column in top command
suggestion of ~300b, but given
architectures tend to end up with very different codegen per arch and
compilers used, i'm always curious which arch and which compiler
(version) was used to obtain the alleged results. Guess you target
chris with gcc-12-ish?
Stating the target arch usually allows us a rough estimate
about overall impact on other arches.
I’m giving you the arch and compiler information below.
C Compiler: gcc -> gcc-10*
ARCH x86_64
Many thanks in advance and cheers,
Bernhard
Br-Matthew
From: Bernhard Reutner-Fischer
Sent: Thursday, February 15, 2024 9:46 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko'
; busybox@busybox.net
; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Wed, 14 Feb 2024 14:05:15 +
Matthew Chae 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
Sent: Tuesday, February 13, 2024 7:16 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko'
; busybox@busybox.net
; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Mon, 5 Feb 2024 09:56:20 +
Matthew Chae 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
Sent: Thursday, November 23, 2023 6:10 PM
To: 'Denys Vlasenko' ; Matthew Chae
Cc: busybox@busybox.net ; Christopher Wong
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 > 9.
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
-
Registe
Re: fix large PID overflow their column in top command
ct on other arches.
I’m giving you the arch and compiler information below.
C Compiler: gcc -> gcc-10*
ARCH x86_64
Many thanks in advance and cheers,
Bernhard
Br-Matthew
From: Bernhard Reutner-Fischer
Sent: Thursday, February 15, 2024 9:46 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko'
; busybox@busybox.net
; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Wed, 14 Feb 2024 14:05:15 +
Matthew Chae 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
Sent: Tuesday, February 13, 2024 7:16 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko'
; busybox@busybox.net
; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Mon, 5 Feb 2024 09:56:20 +
Matthew Chae 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
Sent: Thursday, November 23, 2023 6:10 PM
To: 'Denys Vlasenko' ; Matthew Chae
Cc: busybox@busybox.net ; Christopher Wong
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 > 9.
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)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
, February 15, 2024 9:46 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight ; 'Denys
Vlasenko' ; busybox@busybox.net ; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Wed, 14 Feb 2024 14:05:15 +
Matthew Chae 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_list14061765+359
.rodata99721 99724 +3
--
(add/remove: 0/0 grow/shrink: 2/0 up/down: 362/0) Total: 362 bytes
textdata 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
Sent: Tuesday, February 13, 2024 7:16 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight ; 'Denys
Vlasenko' ; busybox@busybox.net ; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Mon, 5 Feb 2024 09:56:20 +
Matthew Chae 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
Sent: Thursday, November 23, 2023 6:10 PM
To: 'Denys Vlasenko' ; Matthew Chae
Cc: busybox@busybox.net ; Christopher Wong
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 > 9.
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)
From 5a1d02b8b0dcfb49be2494aa58db1895729e44c3 Mon Sep 17 00:00:00 2001
From: Matthew Chae
Date: Thu, 29 Feb 2024 15:09:02 +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 a
Re: fix large PID overflow their column in top command
Hi Sukyoung!
please don't top-post
On Mon, 19 Feb 2024 10:22:05 +
Matthew Chae wrote:
> 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?
Well, 135b is better than the initial 360b :)
> Can you take a look at these attachments?
I'd love to.
But it doesn't apply, unfortunately:
$ git am -s 0004-Allocate-PID-PPID-space-dynamically-in-top-command.patch
Applying: Allocate PID/PPID space dynamically in top command
error: patch failed: procps/top.c:637
error: procps/top.c: patch does not apply
Patch failed at 0001 Allocate PID/PPID space dynamically in top command
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
$ git am --abort
$ patch -p1 -i 0004-Allocate-PID-PPID-space-dynamically-in-top-command.patch
patching file procps/top.c
Hunk #1 FAILED at 637.
Hunk #2 FAILED at 696.
Hunk #3 FAILED at 709.
3 out of 3 hunks FAILED -- saving rejects to file procps/top.c.rej
So I'd have to manually fiddle the patch to apply, which i honestly
don't have time for ATM, as much as i love code-golf in general.
Furthermore (and i'm about to update https://busybox.net/developer
accordingly), for legal reasons, we require a Signed-off-by, as
detailed in
https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=sign%20off#sign-your-work-the-developer-s-certificate-of-origin
so please check you legal department (which should be fine for axis)
and mark your contributions accordingly by 'git commit -s ...' iff this
is in line with your legal situation (again, axis legal will most
likely understand what this is about without much further ado, AFAIK).
>
> PS:
> The function of count_digits() is implemented inside of
> display_process_list().
> To reduce the size, strlen() is not used.
Did you look if manually outlining count_digits() like you did in the
previous version is beneficial?
And, did you check my previous question if it is better to use the
manual buf "painting", perusing in this case pid_len (for the
compile-time constant 6 as it is now) and ppid_len (ditto), and, for
your other patch on top, username_len (for the current compile-time
constant 8)? The loop to determine the max {,p}pid len is not for free
of course, so it's okish if that manifests size-wise.
PS: 135b is better than the initial suggestion of ~300b, but given
architectures tend to end up with very different codegen per arch and
compilers used, i'm always curious which arch and which compiler
(version) was used to obtain the alleged results. Guess you target
chris with gcc-12-ish?
Stating the target arch usually allows us a rough estimate
about overall impact on other arches.
Many thanks in advance and cheers,
Bernhard
>
> Br-Matthew
>
>
> From: Bernhard Reutner-Fischer
> Sent: Thursday, February 15, 2024 9:46 PM
> To: Matthew Chae
> Cc: rep.dot@gmail.com ; David Laight
> ; 'Denys Vlasenko' ;
> busybox@busybox.net ; Christopher Wong
>
> Subject: Re: fix large PID overflow their column in top command
>
> On Wed, 14 Feb 2024 14:05:15 +
> Matthew Chae 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_list14061765+359
> .rodata99721 99724 +3
> --
> (add/remove: 0/0 grow/shrink: 2/0 up/down: 362/0) Total: 362 bytes
>textdata 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))
> th
Re: fix large PID overflow their column in top command
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
Sent: Thursday, February 15, 2024 9:46 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko' ;
busybox@busybox.net ; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Wed, 14 Feb 2024 14:05:15 +
Matthew Chae 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_list14061765+359
.rodata99721 99724 +3
--
(add/remove: 0/0 grow/shrink: 2/0 up/down: 362/0) Total: 362 bytes
textdata 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
> Sent: Tuesday, February 13, 2024 7:16 PM
> To: Matthew Chae
> Cc: rep.dot@gmail.com ; David Laight
> ; 'Denys Vlasenko' ;
> busybox@busybox.net ; Christopher Wong
>
> Subject: Re: fix large PID overflow their column in top command
>
> On Mon, 5 Feb 2024 09:56:20 +
> Matthew Chae 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
> > Sent: Thursday, November 23, 2023 6:10 PM
> > To: 'Denys Vlasenko' ; Matthew Chae
> >
> > Cc: busybox@busybox.net ; Christopher Wong
> >
> > 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 > 9.
> >
> > 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
Re: fix large PID overflow their column in top command
On Wed, 14 Feb 2024 14:05:15 +
Matthew Chae 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_list14061765+359
.rodata99721 99724 +3
--
(add/remove: 0/0 grow/shrink: 2/0 up/down: 362/0) Total: 362 bytes
textdata bss dec hex filename
1009548 165071840 1027895 faf37 busybox_old
1009910 165071840 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
> Sent: Tuesday, February 13, 2024 7:16 PM
> To: Matthew Chae
> Cc: rep.dot@gmail.com ; David Laight
> ; 'Denys Vlasenko' ;
> busybox@busybox.net ; Christopher Wong
>
> Subject: Re: fix large PID overflow their column in top command
>
> On Mon, 5 Feb 2024 09:56:20 +
> Matthew Chae 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
> > Sent: Thursday, November 23, 2023 6:10 PM
> > To: 'Denys Vlasenko' ; Matthew Chae
> >
> > Cc: busybox@busybox.net ; Christopher Wong
> >
> > 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 > 9.
> >
> > 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)
>
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
Hi Bernhard,
I'm sending new patch and the result of bloatcheck.
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
Sent: Tuesday, February 13, 2024 7:16 PM
To: Matthew Chae
Cc: rep.dot@gmail.com ; David Laight
; 'Denys Vlasenko' ;
busybox@busybox.net ; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Mon, 5 Feb 2024 09:56:20 +
Matthew Chae 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
> Sent: Thursday, November 23, 2023 6:10 PM
> To: 'Denys Vlasenko' ; Matthew Chae
>
> Cc: busybox@busybox.net ; Christopher Wong
>
> 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 > 9.
>
> 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
Description: bloatcheck_top
From a3d8d00fec2514b52bd253911e0fab2205c4e1ef Mon Sep 17 00:00:00 2001
From: Matthew Chae
Date: Wed, 14 Feb 2024 14:41:18 +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
Signed-off-by: Matthew(Sukyoung) Chae
---
procps/top.c | 83
1 file changed, 51 insertions(+), 32 deletions(-)
diff --git a/procps/top.c b/procps/top.c
index 09d31c673..4f5f0e4a0 100644
--- a/procps/top.c
+++ b/procps/top.c
@@ -602,6 +602,22 @@ static unsigned long display_header(int scr_width, int *lines_rem_p)
return meminfo[MI_MEMTOTAL];
}
+static int count_digits(unsigned num)
+{
+ int cnt = 0;
+
+ if (num < 10)
+ return 1;
+
+ while (num > 0)
+ {
+ num /= 10;
+ cnt++;
+ }
+
+ return cnt;
+}
+
static NOINLINE void display_process_list(int lines_rem, int scr_width)
{
enum {
@@ -619,6 +635,30 @@ static NOINLINE void display_process_list(int lines_rem, int scr_width)
unsigned busy_jifs;
#endif
+#define PID_STR "PID"
+#define PPID_STR "PPID"
+#define DEFAULT_DIGIT_MIN 5
+
+ char top_title_str[scr_width + 1];
+ int lines = (lines_rem - 1);
+ unsigned pid_max = 0;
+ unsigned ppid_max = 0;
+ unsigned pid_digits_max = DEFAULT_DIGIT_MIN;
+ unsigned ppid_digits_max = DEFAULT_DIGIT_MIN;
+ top_status_t *s_tmp = top + G_scroll_ofs;
+
+ if (lines > ntop - G_scroll_ofs)
+ lines = ntop - G_scroll_ofs;
+
+ while (--li
Re: fix large PID overflow their column in top command
On Mon, 5 Feb 2024 09:56:20 +
Matthew Chae 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
> Sent: Thursday, November 23, 2023 6:10 PM
> To: 'Denys Vlasenko' ; Matthew Chae
>
> Cc: busybox@busybox.net ; Christopher Wong
>
> 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 > 9.
>
> 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)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
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?
(0002-Allocate-PID-PPID-space-dynamically-in-top-command.patch)
BR-Matthew Chae
From: David Laight
Sent: Thursday, November 23, 2023 6:10 PM
To: 'Denys Vlasenko' ; Matthew Chae
Cc: busybox@busybox.net ; Christopher Wong
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 > 9.
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)
From 4d24591cabe7ddd2305cbe171a30ac33f10234eb Mon Sep 17 00:00:00 2001
From: Matthew Chae
Date: Mon, 5 Feb 2024 09:23:59 +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
Signed-off-by: Matthew(Sukyoung) Chae
---
procps/top.c | 80 +---
1 file changed, 51 insertions(+), 29 deletions(-)
diff --git a/procps/top.c b/procps/top.c
index ff77542..ed4fae4 100644
--- a/procps/top.c
+++ b/procps/top.c
@@ -602,6 +602,22 @@ static unsigned long display_header(int scr_width, int *lines_rem_p)
return meminfo[MI_MEMTOTAL];
}
+static int count_digits(unsigned num)
+{
+ int cnt = 0;
+
+ if (num < 10)
+ return 1;
+
+ while (num > 0)
+ {
+ num /= 10;
+ cnt++;
+ }
+
+ return cnt;
+}
+
static NOINLINE void display_process_list(int lines_rem, int scr_width)
{
enum {
@@ -619,12 +635,40 @@ static NOINLINE void display_process_list(int lines_rem, int scr_width)
unsigned busy_jifs;
#endif
+#define PID_STR "PID"
+#define PPID_STR "PPID"
+#define DEFAULT_DIGIT_MIN 5
+
+ char top_title_str[scr_width + 1];
+ int lines = (lines_rem - 1);
+ unsigned pid_max = 0;
+ unsigned ppid_max = 0;
+ unsigned pid_digits_max = DEFAULT_DIGIT_MIN;
+ unsigned ppid_digits_max = DEFAULT_DIGIT_MIN;
+ 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++;
+ }
+
+ pid_digits_max = MAX(pid_digits_max, count_digits(pid_max));
+ ppid_digits_max = MAX(ppid_digits_max, count_digits(ppid_max));
+
/* what info of the processes is shown */
- printf(OPT_BATCH_MODE ? "%.*s" : ESC"[7m" "%.*s" ESC"[m", scr_width,
- " PID PPID USER STAT VSZ %VSZ"
+ snprintf(top_title_str, scr_width + 1,
+ "%*s%s %*s%s %s",
+ pid_digits_max - (int)strlen(PID_STR), " ", PID_STR,
+ ppid_digits_max - (int)strlen(PPID_STR), " ", PPID_STR,
+ "USER STAT VSZ %VSZ"
IF_FEATURE_TOP_SMP_PROCESS(" CPU")
IF_FEATURE_TOP_CPU_USAGE_PERCENTAGE(" %CPU")
" COMMAND");
+ printf(OPT_BATCH_MODE ? "%.*s" : ESC"[7m" "%.*s" ESC"[m", scr_width, top_title_str);
lines_rem--;
#if ENABLE_FEATURE_TOP_DECIMALS
@@ -689,8 +733,6 @@ static NOINLINE void display_process_list(int lines_rem, in
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 > 9.
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)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
On Fri, Nov 10, 2023 at 4:22 PM Matthew Chae wrote:
> Hi Denys,
>
> I'm sending the patch what I've mentioned. Can you take a look at this?
> If I don't understand correctly what you said before, let me know.
> > The max PID value can have 7 digits as the 4194304.
> > Busybox v1.36 released a new patch to handle large PID value in top command.
> > However, it still causes misalignment between the title and the number for
> > 7 digits or 6 digits PID.
> > 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.
> > Plus, the large PID causes that user name can't be displayed up to 8
> > characters and result in the truncation of less than 8 characters.
> >
> > I can contribute new patch which allocates appropriate space for the number
> > of digits in the PID and PPID to represent the values in each column
> > correctly.
> > This can make alignment between the title and the number for 7 digits PID
> > and 5 digits PID and also display user name up to 8 characters.
> >
> > What do you think? If you agree, I will send a patch.
>
> Absolutely, please send a patch. Would be interesting to see a good solution.
+ 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 > 9.
Ideally, the code should handle any pids correctly.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
Hi Denys, A gentle reminder that when you're available, can you take a look at this? Have a great day, Matthew Chae From: Matthew Chae Sent: Friday, November 10, 2023 4:22 PM To: Denys Vlasenko Cc: busybox@busybox.net ; Christopher Wong Subject: Re: fix large PID overflow their column in top command Hi Denys, I'm sending the patch what I've mentioned. Can you take a look at this? If I don't understand correctly what you said before, let me know. Have a nice weekend, Matthew Chae From: busybox on behalf of Matthew Chae Sent: Wednesday, November 8, 2023 5:33 PM To: Denys Vlasenko Cc: busybox@busybox.net ; Christopher Wong Subject: Re: fix large PID overflow their column in top command Hi Denys, Ok. Good to hear. If I understand correctly, will you first review my patch and then give me Morris's account to commit the change? Then, let me send a patch soon. Br-Matthew From: Denys Vlasenko Sent: Wednesday, November 8, 2023 12:35 PM To: Matthew Chae Cc: busybox@busybox.net ; Christopher Wong Subject: Re: fix large PID overflow their column in top command On Wed, Nov 1, 2023 at 10:18 AM Matthew Chae wrote: > > Hi maintainer, > > I'd like to hear your opinion regarding large PID can overflow its column in > top command. > > The max PID value can have 7 digits as the 4194304. > Busybox v1.36 released a new patch to handle large PID value in top command. > However, it still causes misalignment between the title and the number for 7 > digits or 6 digits PID. > 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. > Plus, the large PID causes that user name can't be displayed up to 8 > characters and result in the truncation of less than 8 characters. > > I can contribute new patch which allocates appropriate space for the number > of digits in the PID and PPID to represent the values in each column > correctly. > This can make alignment between the title and the number for 7 digits PID and > 5 digits PID and also display user name up to 8 characters. > > What do you think? If you agree, I will send a patch. Absolutely, please send a patch. Would be interesting to see a good solution. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
Hi Denys,
I'm sending the patch what I've mentioned. Can you take a look at this?
If I don't understand correctly what you said before, let me know.
Have a nice weekend,
Matthew Chae
From: busybox on behalf of Matthew Chae
Sent: Wednesday, November 8, 2023 5:33 PM
To: Denys Vlasenko
Cc: busybox@busybox.net ; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
Hi Denys,
Ok. Good to hear.
If I understand correctly, will you first review my patch and then give me
Morris's account to commit the change?
Then, let me send a patch soon.
Br-Matthew
From: Denys Vlasenko
Sent: Wednesday, November 8, 2023 12:35 PM
To: Matthew Chae
Cc: busybox@busybox.net ; Christopher Wong
Subject: Re: fix large PID overflow their column in top command
On Wed, Nov 1, 2023 at 10:18 AM Matthew Chae wrote:
>
> Hi maintainer,
>
> I'd like to hear your opinion regarding large PID can overflow its column in
> top command.
>
> The max PID value can have 7 digits as the 4194304.
> Busybox v1.36 released a new patch to handle large PID value in top command.
> However, it still causes misalignment between the title and the number for 7
> digits or 6 digits PID.
> 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.
> Plus, the large PID causes that user name can't be displayed up to 8
> characters and result in the truncation of less than 8 characters.
>
> I can contribute new patch which allocates appropriate space for the number
> of digits in the PID and PPID to represent the values in each column
> correctly.
> This can make alignment between the title and the number for 7 digits PID and
> 5 digits PID and also display user name up to 8 characters.
>
> What do you think? If you agree, I will send a patch.
Absolutely, please send a patch. Would be interesting to see a good solution.
From 42cafd2bbbdc37ef19ddec7f09311f13d83cc957 Mon Sep 17 00:00:00 2001
From: Matthew Chae
Date: Fri, 10 Nov 2023 15:18:52 +0100
Subject: [PATCH] fix large PID overflow their column 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 appropriate space based on the
max number of PID to accurately represent the values in each column,
also ensuring that user names are displayed without truncation, up to 8
characters.
Signed-off-by: Matthew Chae
---
procps/top.c | 61 +---
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/procps/top.c b/procps/top.c
index 6d25d9633..f158c8ae1 100644
--- a/procps/top.c
+++ b/procps/top.c
@@ -604,6 +604,8 @@ static unsigned long display_header(int scr_width, int *lines_rem_p)
static NOINLINE void display_process_list(int lines_rem, int scr_width)
{
+#define PID_DIGITS_MAX 7 /* 4194304 */
+
enum {
BITS_PER_INT = sizeof(int) * 8
};
@@ -637,12 +639,30 @@ typedef struct { unsigned quot, rem; } bb_div_t;
# define FMT "%4u%%"
#endif
+ char pid_buf[PID_DIGITS_MAX + 1];
+ char top_title_str[60];
+ unsigned int pid_digits_num;
+ FILE *fp;
+
+ fp = xfopen_for_read("/proc/sys/kernel/pid_max");
+ if (!fgets(pid_buf, PID_DIGITS_MAX + 1, fp)) {
+ fclose(fp);
+ bb_simple_perror_msg_and_die("can't read standard input");
+ }
+ fclose(fp);
+
+ if (strncmp(pid_buf, "32768", 5) == 0)
+ pid_digits_num = 5;
+ else
+ pid_digits_num = PID_DIGITS_MAX;
+
/* what info of the processes is shown */
- printf(OPT_BATCH_MODE ? "%.*s" : ESC"[7m" "%.*s" ESC"[m", scr_width,
- " PID PPID USER STAT VSZ %VSZ"
- IF_FEATURE_TOP_SMP_PROCESS(" CPU")
- IF_FEATURE_TOP_CPU_USAGE_PERCENTAGE(" %CPU")
- " COMMAND");
+ sprintf(top_title_str, "%*sPID %*sPPID USER STAT VSZ %VSZ",
+ pid_digits_num - 3, " ", pid_digits_num - 4, " ");
+ strcat(top_title_str, IF_FEATURE_TOP_SMP_PROCESS(" CPU")
+ IF_FEATURE_TOP_CPU_USAGE_PERCENTAGE(" %CPU") " COMMAND");
+ printf(OPT_BATCH_MODE ? "%.*s" : ESC"[7m" "%.*s" ESC"[m", scr_width, top_title_str);
+
lines_rem--;
/*
@@ -696,8 +716,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,36 +727,15 @@ 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
Re: fix large PID overflow their column in top command
Hi Denys, Ok. Good to hear. If I understand correctly, will you first review my patch and then give me Morris's account to commit the change? Then, let me send a patch soon. Br-Matthew From: Denys Vlasenko Sent: Wednesday, November 8, 2023 12:35 PM To: Matthew Chae Cc: busybox@busybox.net ; Christopher Wong Subject: Re: fix large PID overflow their column in top command On Wed, Nov 1, 2023 at 10:18 AM Matthew Chae wrote: > > Hi maintainer, > > I'd like to hear your opinion regarding large PID can overflow its column in > top command. > > The max PID value can have 7 digits as the 4194304. > Busybox v1.36 released a new patch to handle large PID value in top command. > However, it still causes misalignment between the title and the number for 7 > digits or 6 digits PID. > 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. > Plus, the large PID causes that user name can't be displayed up to 8 > characters and result in the truncation of less than 8 characters. > > I can contribute new patch which allocates appropriate space for the number > of digits in the PID and PPID to represent the values in each column > correctly. > This can make alignment between the title and the number for 7 digits PID and > 5 digits PID and also display user name up to 8 characters. > > What do you think? If you agree, I will send a patch. Absolutely, please send a patch. Would be interesting to see a good solution. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: fix large PID overflow their column in top command
On Wed, Nov 1, 2023 at 10:18 AM Matthew Chae wrote: > > Hi maintainer, > > I'd like to hear your opinion regarding large PID can overflow its column in > top command. > > The max PID value can have 7 digits as the 4194304. > Busybox v1.36 released a new patch to handle large PID value in top command. > However, it still causes misalignment between the title and the number for 7 > digits or 6 digits PID. > 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. > Plus, the large PID causes that user name can't be displayed up to 8 > characters and result in the truncation of less than 8 characters. > > I can contribute new patch which allocates appropriate space for the number > of digits in the PID and PPID to represent the values in each column > correctly. > This can make alignment between the title and the number for 7 digits PID and > 5 digits PID and also display user name up to 8 characters. > > What do you think? If you agree, I will send a patch. Absolutely, please send a patch. Would be interesting to see a good solution. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
