Re: git: 62e6ca0f07e4 - main - ps(1): clean up after swapout removal
- In reply to: Konstantin Belousov : "git: 62e6ca0f07e4 - main - ps(1): clean up after swapout removal"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Thu, 14 Nov 2024 02:21:24 UTC
On Sat, Nov 9, 2024 at 2:25 PM Konstantin Belousov <kib@freebsd.org> wrote:
>
> The branch main has been updated by kib:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=62e6ca0f07e448da27cb2cc8165e749e7fdfcd7e
>
> commit 62e6ca0f07e448da27cb2cc8165e749e7fdfcd7e
> Author: Konstantin Belousov <kib@FreeBSD.org>
> AuthorDate: 2024-11-09 01:37:07 +0000
> Commit: Konstantin Belousov <kib@FreeBSD.org>
> CommitDate: 2024-11-09 17:22:42 +0000
>
> ps(1): clean up after swapout removal
>
> The process flag P_INMEM is always set. Eliminate all checks for the
> bit. Also eliminate LAZY_PS define and code covered by it: we do not
> have an u-area for long time, and it cannot be swapped out.
>
> Also eliminate setting controlled by the '-f' switch, but accept it for
> backward compatibility.
>
> The 'W' process secondary state (swapped out) is impossible, stop
> calculating it.
>
> Reviewed by: markj
> Sponsored by: The FreeBSD Foundation
> Differential revision: https://reviews.freebsd.org/D47492
> ---
> bin/ps/Makefile | 7 -------
> bin/ps/print.c | 6 +-----
> bin/ps/ps.1 | 9 +--------
> bin/ps/ps.c | 41 ++++++++---------------------------------
> 4 files changed, 10 insertions(+), 53 deletions(-)
>
> diff --git a/bin/ps/Makefile b/bin/ps/Makefile
> index a25b6a796ed0..71973b34dd24 100644
> --- a/bin/ps/Makefile
> +++ b/bin/ps/Makefile
> @@ -2,13 +2,6 @@ PACKAGE=runtime
> PROG= ps
> SRCS= fmt.c keyword.c nlist.c print.c ps.c
>
> -#
> -# To support "lazy" ps for non root/wheel users
> -# add -DLAZY_PS to the cflags. This helps
> -# keep ps from being an unnecessary load
> -# on large systems.
> -#
> -CFLAGS+=-DLAZY_PS
> LIBADD= m kvm jail xo
>
> .include <bsd.prog.mk>
> diff --git a/bin/ps/print.c b/bin/ps/print.c
> index a3423d8b3956..59631fb66a10 100644
> --- a/bin/ps/print.c
> +++ b/bin/ps/print.c
> @@ -253,8 +253,6 @@ state(KINFO *k, VARENT *ve __unused)
> *cp = '?';
> }
> cp++;
> - if (!(flag & P_INMEM))
> - *cp++ = 'W';
> if (k->ki_p->ki_nice < NZERO || k->ki_p->ki_pri.pri_class == PRI_REALTIME)
> *cp++ = '<';
> else if (k->ki_p->ki_nice > NZERO || k->ki_p->ki_pri.pri_class == PRI_IDLE)
> @@ -633,7 +631,7 @@ getpcpu(const KINFO *k)
> #define fxtofl(fixpt) ((double)(fixpt) / fscale)
>
> /* XXX - I don't like this */
> - if (k->ki_p->ki_swtime == 0 || (k->ki_p->ki_flag & P_INMEM) == 0)
> + if (k->ki_p->ki_swtime == 0)
> return (0.0);
> if (rawcpu)
> return (100.0 * fxtofl(k->ki_p->ki_pctcpu));
> @@ -661,8 +659,6 @@ getpmem(KINFO *k)
> if (failure)
> return (0.0);
>
> - if ((k->ki_p->ki_flag & P_INMEM) == 0)
> - return (0.0);
> /* XXX want pmap ptpages, segtab, etc. (per architecture) */
> /* XXX don't have info about shared */
> fracmem = ((double)k->ki_p->ki_rssize) / mempages;
> diff --git a/bin/ps/ps.1 b/bin/ps/ps.1
> index 828239fd2ba9..8ece5b1bbfad 100644
> --- a/bin/ps/ps.1
> +++ b/bin/ps/ps.1
> @@ -159,9 +159,6 @@ does not imply
> but works well with it.
> .It Fl e
> Display the environment as well.
> -.It Fl f
> -Show command-line and environment information about swapped out processes.
> -This option is honored only if the UID of the user is 0.
> .It Fl G
> Display information about processes which are running with the specified
> real group IDs.
> @@ -358,9 +355,7 @@ the include file
> .It Dv "P_TOTAL_STOP" Ta No "0x02000000" Ta "Stopped for system suspend"
> .It Dv "P_INEXEC" Ta No "0x04000000" Ta Process is in Xr execve 2
> .It Dv "P_STATCHILD" Ta No "0x08000000" Ta "Child process stopped or exited"
> -.It Dv "P_INMEM" Ta No "0x10000000" Ta "Loaded into memory"
> -.It Dv "P_SWAPPINGOUT" Ta No "0x20000000" Ta "Process is being swapped out"
> -.It Dv "P_SWAPPINGIN" Ta No "0x40000000" Ta "Process is being swapped in"
> +.It Dv "P_INMEM" Ta No "0x10000000" Ta "Always set, unused"
> .It Dv "P_PPTRACE" Ta No "0x80000000" Ta "Vforked child issued ptrace(PT_TRACEME)"
> .El
> .It Cm flags2
> @@ -491,8 +486,6 @@ The process is a session leader.
> The process' parent is suspended during a
> .Xr vfork 2 ,
> waiting for the process to exec or exit.
> -.It Li W
> -The process is swapped out.
> .It Li X
> The process is being traced or debugged.
> .El
> diff --git a/bin/ps/ps.c b/bin/ps/ps.c
> index b0af2bdf37ca..49c69bb76084 100644
> --- a/bin/ps/ps.c
> +++ b/bin/ps/ps.c
> @@ -68,14 +68,6 @@
> #define W_SEP " \t" /* "Whitespace" list separators */
> #define T_SEP "," /* "Terminate-element" list separators */
>
> -#ifdef LAZY_PS
> -#define DEF_UREAD 0
> -#define OPT_LAZY_f "f"
> -#else
> -#define DEF_UREAD 1 /* Always do the more-expensive read. */
> -#define OPT_LAZY_f /* I.e., the `-f' option is not added. */
> -#endif
> -
> /*
> * isdigit takes an `int', but expects values in the range of unsigned char.
> * This wrapper ensures that values from a 'char' end up in the correct range.
> @@ -92,7 +84,6 @@ int showthreads; /* will threads be shown? */
>
> struct velisthead varlist = STAILQ_HEAD_INITIALIZER(varlist);
>
> -static int forceuread = DEF_UREAD; /* Do extra work to get u-area. */
> static kvm_t *kd;
> static int needcomm; /* -o "command" */
> static int needenv; /* -e */
> @@ -154,7 +145,7 @@ static char vfmt[] = "pid,state,time,sl,re,pagein,vsz,rss,lim,tsiz,"
> "%cpu,%mem,command";
> static char Zfmt[] = "label";
>
> -#define PS_ARGS "AaCcD:de" OPT_LAZY_f "G:gHhjJ:LlM:mN:O:o:p:rSTt:U:uvwXxZ"
> +#define PS_ARGS "AaCcD:defG:gHhjJ:LlM:mN:O:o:p:rSTt:U:uvwXxZ"
>
> int
> main(int argc, char *argv[])
> @@ -272,12 +263,9 @@ main(int argc, char *argv[])
> case 'e': /* XXX set ufmt */
> needenv = 1;
> break;
> -#ifdef LAZY_PS
> case 'f':
> - if (getuid() == 0 || getgid() == 0)
> - forceuread = 1;
> + /* compat */
> break;
> -#endif
> case 'G':
> add_list(&gidlist, optarg);
> xkeep_implied = 1;
> @@ -1276,31 +1264,21 @@ fmt(char **(*fn)(kvm_t *, const struct kinfo_proc *, int), KINFO *ki,
> return (s);
> }
>
> -#define UREADOK(ki) (forceuread || (ki->ki_p->ki_flag & P_INMEM))
> -
> static void
> saveuser(KINFO *ki)
> {
> char tdname[COMMLEN + 1];
> char *argsp;
>
> - if (ki->ki_p->ki_flag & P_INMEM) {
> - /*
> - * The u-area might be swapped out, and we can't get
> - * at it because we have a crashdump and no swap.
> - * If it's here fill in these fields, otherwise, just
> - * leave them 0.
> - */
> - ki->ki_valid = 1;
> - } else
> - ki->ki_valid = 0;
> + ki->ki_valid = 1;
> +
> /*
> * save arguments if needed
> */
> if (needcomm) {
> if (ki->ki_p->ki_stat == SZOMB) {
> ki->ki_args = strdup("<defunct>");
> - } else if (UREADOK(ki) || (ki->ki_p->ki_args != NULL)) {
Apparently this is missing an explicit check of ki->ki_p->ki_flag,
causing the processes in square brackets be printed within
parentheses, that is, taking the else path below, and making test:
https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/lastCompletedBuild/testReport/bin.pkill/pgrep-_s_test/main/
fail.
I have already sent a message via Phabricator, but I am unsure if it
has visibility there after the commit.
I apologize if this is something that's already known.
Thank you!
> + } else if (ki->ki_p->ki_args != NULL) {
> (void)snprintf(tdname, sizeof(tdname), "%s%s",
> ki->ki_p->ki_tdname, ki->ki_p->ki_moretdname);
> ki->ki_args = fmt(kvm_getargv, ki,
> @@ -1315,11 +1293,8 @@ saveuser(KINFO *ki)
> ki->ki_args = NULL;
> }
> if (needenv) {
> - if (UREADOK(ki))
> - ki->ki_env = fmt(kvm_getenvv, ki,
> - (char *)NULL, (char *)NULL, 0);
> - else
> - ki->ki_env = strdup("()");
> + ki->ki_env = fmt(kvm_getenvv, ki, (char *)NULL,
> + (char *)NULL, 0);
> if (ki->ki_env == NULL)
> xo_errx(1, "malloc failed");
> } else {
> @@ -1479,7 +1454,7 @@ pidmax_init(void)
> static void __dead2
> usage(void)
> {
> -#define SINGLE_OPTS "[-aCcde" OPT_LAZY_f "HhjlmrSTuvwXxZ]"
> +#define SINGLE_OPTS "[-aCcdeHhjlmrSTuvwXxZ]"
>
> xo_error("%s\n%s\n%s\n%s\n%s\n",
> "usage: ps [--libxo] " SINGLE_OPTS " [-O fmt | -o fmt]",
--
Jose Luis Duran