On Wed, 14 Feb 2024 14:05:15 +0000
Matthew Chae <[email protected]> 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 <[email protected]>
> Sent: Tuesday, February 13, 2024 7:16 PM
> To: Matthew Chae <[email protected]>
> Cc: [email protected] <[email protected]>; David Laight
> <[email protected]>; 'Denys Vlasenko' <[email protected]>;
> [email protected] <[email protected]>; Christopher Wong
> <[email protected]>
> Subject: Re: fix large PID overflow their column in top command
>
> On Mon, 5 Feb 2024 09:56:20 +0000
> Matthew Chae <[email protected]> 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 <[email protected]>
> > Sent: Thursday, November 23, 2023 6:10 PM
> > To: 'Denys Vlasenko' <[email protected]>; Matthew Chae
> > <[email protected]>
> > Cc: [email protected] <[email protected]>; Christopher Wong
> > <[email protected]>
> > 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)
>
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
