Page MenuHomeFreeBSD
Differential D31986

Convert vm_page_alloc() callers to use vm_page_alloc_noobj().
ClosedPublic
Actions

Authored by markj on Sep 16 2021, 4:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 11, 8:55 PM
Unknown Object (File)
Wed, Apr 2, 8:57 AM
Unknown Object (File)
Tue, Apr 1, 8:32 PM
Unknown Object (File)
Mon, Mar 31, 11:00 AM
Unknown Object (File)
Thu, Mar 27, 9:33 PM
Unknown Object (File)
Sat, Mar 22, 11:44 PM
Unknown Object (File)
Fri, Mar 14, 4:19 PM
Unknown Object (File)
Mar 8 2025, 2:32 PM
View All Files
Subscribers
andrew
emaste
• hselasky
imp
jhibbits
linuxkpi

Details

Reviewers
alc
kib
manu
• hselasky
Commits
rGa4667e09e652: Convert vm_page_alloc() callers to use vm_page_alloc_noobj().
Summary

Remove page zeroing code from consumers and stop specifying
VM_ALLOC_NOOBJ. In a few places I also converted an allocation loop to
use VM_ALLOC_WAITOK.

Similarly, convert vm_page_alloc_domain() callers.

Note that callers are now responsible for assigning the pindex.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41593
Build 38482: arc lint + arc unit

Event Timeline

markj created this revision.Sep 16 2021, 4:56 PM
Herald added a reviewer: manu. · View Herald TranscriptSep 16 2021, 4:56 PM
Herald added subscribers: linuxkpi, emaste, jhibbits and 2 others. · View Herald Transcript
markj requested review of this revision.Sep 16 2021, 4:56 PM
markj added a parent revision: D31985: vm_page: Add a new page allocator interface for anonymous pages.
Harbormaster completed remote builds in B41545: Diff 95254.Sep 16 2021, 4:56 PM
kib added inline comments.Sep 16 2021, 8:08 PM
sys/i386/i386/pmap.c
2088

Eliminate m while there?

• hselasky accepted this revision.Sep 17 2021, 7:43 AM
• hselasky added a subscriber: • hselasky.

Looks good from my point of view.

This revision is now accepted and ready to land.Sep 17 2021, 7:43 AM
markj updated this revision to Diff 95284.Sep 17 2021, 1:16 PM
markj marked an inline comment as done.

Eliminate a local var in the i386 pmap.

This revision now requires review to proceed.Sep 17 2021, 1:16 PM
Harbormaster completed remote builds in B41563: Diff 95284.Sep 17 2021, 1:16 PM
• hselasky accepted this revision.Sep 17 2021, 1:47 PM
This revision is now accepted and ready to land.Sep 17 2021, 1:47 PM
kib accepted this revision.Sep 17 2021, 5:53 PM
markj updated this revision to Diff 95316.Sep 17 2021, 7:09 PM

Drop unused VM_ALLOC_NOBUSY from consumers.

This revision now requires review to proceed.Sep 17 2021, 7:09 PM
Harbormaster completed remote builds in B41575: Diff 95316.Sep 17 2021, 7:09 PM
alc added inline comments.Sep 18 2021, 6:01 PM
sys/amd64/amd64/pmap.c
11390–11392

This doesn't look right.

markj updated this revision to Diff 95343.Sep 18 2021, 6:14 PM
markj marked an inline comment as done.

Fix a couple of callers that weren't converted properly.

Harbormaster completed remote builds in B41593: Diff 95343.Sep 18 2021, 6:14 PM
markj updated this revision to Diff 96117.Oct 1 2021, 9:48 PM

Tinderbox fixes.

markj mentioned this in D28810: Let the VM page allocator handle page zeroing.Oct 1 2021, 9:49 PM
Harbormaster completed remote builds in B41909: Diff 96117.Oct 1 2021, 9:49 PM
markj added a child revision: D32005: Introduce vm_page_alloc_noobj_contig().Oct 1 2021, 9:55 PM
alc added a comment.Oct 2 2021, 7:26 PM

There are a number of places where VM_ALLOC_NORMAL is missing, but implied. And, given its definition (as 0), we have no way of enforcing its use. So, I'm ready to suggest that we simply eliminate it.

markj updated this revision to Diff 96188.Oct 4 2021, 12:02 AM

Drop uses of VM_ALLOC_NORMAL from vm_page_alloc_noobj() and
vm_page_alloc_noobj_domain() callers.

Harbormaster completed remote builds in B41937: Diff 96188.Oct 4 2021, 12:02 AM
kib accepted this revision.Oct 4 2021, 1:41 AM
This revision is now accepted and ready to land.Oct 4 2021, 1:41 AM
alc accepted this revision.Oct 16 2021, 8:37 PM
alc added inline comments.
sys/amd64/amd64/pmap.c
4354

As an aside, this comment should explain why 'pmap' isn't passed to 'pmap_alloc_pt_page'.

4386–4388

As an aside, as above this deserves a comment (as part of a separate patch) so that readers don't think it is an accounting bug.

4392

I feel like I'm missing something here. We only appear to initialize one PTE within the pml5 page and the contents of the rest of the PTEs within the page are unknown.

sys/dev/xen/balloon/balloon.c
237

As an aside, it's curious that we specify "nodump" in the virtio driver but not here.

markj added inline comments.Oct 16 2021, 9:10 PM
sys/amd64/amd64/pmap.c
4354

Is it only because the pmap stats fields are not initialized at this point? If so, I wonder if we should instead initialize them first and count this page (with corresponding changes to pmap_release()).

4386–4388

Here it makes a bit more sense that the page isn't counted.

4392

I think this is a bug.

sys/dev/xen/balloon/balloon.c
237

I can't see a reason for virtio to specify NODUMP: it doesn't call dump_add_page() and I can't see how or why virtio balloon pages would ever be mapped.

kib added inline comments.Oct 16 2021, 9:17 PM
sys/amd64/amd64/pmap.c
4386–4388

I do not see why, it is reasonable to account for all page table pages, even if we have two page tables.

4392

Yes it is bug. Would you add VM_ALLOC_ZERO as part of this patch or should we do a separate patch?

markj added inline comments.Oct 16 2021, 9:49 PM
sys/amd64/amd64/pmap.c
4386–4388

Ah, pmap_resident_count_adj() asserts that the pmap mutex is held, but at this point it is not. And it doesn't seem very useful to manually adjust the count.

alc added inline comments.Oct 16 2021, 11:30 PM
sys/amd64/amd64/pmap.c
4354

No. Think about the early termination optimization in pmap_remove(). For it to work, the root-level PTPs allocated in this function can't be counted in the pmap's resident.

alc added inline comments.Oct 16 2021, 11:53 PM
sys/dev/xen/balloon/balloon.c
237

Yes, you are right. There is no reason for the virtio balloon driver to pass the "no dump" flag.

ehem_freebsd_m5p.com mentioned this in D28982: xen: move common variables and code off of sys/x86/xen/hvm.c.Oct 21 2021, 10:07 PM
Closed by commit rGa4667e09e652: Convert vm_page_alloc() callers to use vm_page_alloc_noobj(). (authored by markj). · Explain WhyNov 3 2021, 4:16 AM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rGa4667e09e652: Convert vm_page_alloc() callers to use vm_page_alloc_noobj()..

Revision Contents
Changeset List

PathSize
sys/
amd64/
amd64/
3 lines
32 lines
6 lines
arm/
arm/
18 lines
arm64/
arm64/
42 lines
6 lines
iommu/
15 lines
compat/
linuxkpi/
common/
src/
2 lines
dev/
drm2/
ttm/
7 lines
ti/
3 lines
virtio/
balloon/
3 lines
xen/
balloon/
16 lines
gntdev/
11 lines
fs/
nfs/
9 lines
nfsclient/
9 lines
i386/
i386/
43 lines
kern/
4 lines
9 lines
5 lines
5 lines
mips/
mips/
4 lines
2 lines
powerpc/
aim/
7 lines
37 lines
booke/
4 lines
9 lines
powerpc/
7 lines
riscv/
riscv/
54 lines
6 lines
vm/
22 lines
6 lines
3 lines

Diff 95343

View Options

sys/amd64/amd64/mp_machdep.c

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

Why? Other uses of strtou*() in the file or across the base don't use macros for integer bases.

usr.bin/xargs/xargs.c:                  Rflag = strtol(optarg, &endptr, 10);
usr.bin/xargs/xargs.c:                  Sflag = strtoul(optarg, &endptr, 10);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

Why allow value larger than RLIMIT_NPROC or _SC_CHILD_MAX? Soft limit would cap number of processes sooner. And kern.maxproc can be larger than "infinity" hard limit.

$ limits -Su 50 sh -c "yes | xargs -n1 -P0 yes foo" >/dev/null &
$ pgrep -f foo | wc -l
       6

$ limits -Su
Resource limits (current):
  maxprocesses-cur          100
$ limits -Hu
Resource limits (current):
  maxprocesses-max         1458
$ sysctl kern.max{proc,users}
kern.maxproc: 1620 # (20 + 16 * maxusers)
kern.maxusers: 100
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

I do not see how nargs is the correct value to use here; it has a different meaning to the program, and can be overridden by other command-line arguments, giving a bizzare behavior change if -n 1 -P 0 are passed in different orders.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

The code form user perspective should behave just like gnu xargs, unless there is a good reason to deviate, which I don't see.

In this particular case, they have "unlimited" for an argument == 0, specified limit for > 0 and error out for non-numbers and negative values. Here negative values are accepted for some reason.

One big not-user-visible difference is the fact that gnu xargs dynamically resizes the process table, while our xargs allocates it upfront. This means we have to have a reasonable limit which can pose as "unlimited" for practical purposes.

getrlimit seen here is fine enough.

However, the code must not complain about passed limit being bigger. It should internally cap it to getrlimit so that user-visible behaviour is identical with gnu in this respect.

As a result the code should explicitly coompare maxprocs to 0 (as opposed to < 1), proclim variable has no use. Fetch rlimit /after/ maxprocs is validated. If (maxprocs == 0 || maxprocs > reslimit.rlim_cur) maxprocs = reslimit.rlim_cur;

getrlimit realistically should never fail, so a failure with err(1, "getrlimit"); should be fine enough for surprising cases.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

sys/amd64/amd64/pmap.c

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

Why? Other uses of strtou*() in the file or across the base don't use macros for integer bases.

usr.bin/xargs/xargs.c:                  Rflag = strtol(optarg, &endptr, 10);
usr.bin/xargs/xargs.c:                  Sflag = strtoul(optarg, &endptr, 10);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

Why allow value larger than RLIMIT_NPROC or _SC_CHILD_MAX? Soft limit would cap number of processes sooner. And kern.maxproc can be larger than "infinity" hard limit.

$ limits -Su 50 sh -c "yes | xargs -n1 -P0 yes foo" >/dev/null &
$ pgrep -f foo | wc -l
       6

$ limits -Su
Resource limits (current):
  maxprocesses-cur          100
$ limits -Hu
Resource limits (current):
  maxprocesses-max         1458
$ sysctl kern.max{proc,users}
kern.maxproc: 1620 # (20 + 16 * maxusers)
kern.maxusers: 100
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

I do not see how nargs is the correct value to use here; it has a different meaning to the program, and can be overridden by other command-line arguments, giving a bizzare behavior change if -n 1 -P 0 are passed in different orders.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

The code form user perspective should behave just like gnu xargs, unless there is a good reason to deviate, which I don't see.

In this particular case, they have "unlimited" for an argument == 0, specified limit for > 0 and error out for non-numbers and negative values. Here negative values are accepted for some reason.

One big not-user-visible difference is the fact that gnu xargs dynamically resizes the process table, while our xargs allocates it upfront. This means we have to have a reasonable limit which can pose as "unlimited" for practical purposes.

getrlimit seen here is fine enough.

However, the code must not complain about passed limit being bigger. It should internally cap it to getrlimit so that user-visible behaviour is identical with gnu in this respect.

As a result the code should explicitly coompare maxprocs to 0 (as opposed to < 1), proclim variable has no use. Fetch rlimit /after/ maxprocs is validated. If (maxprocs == 0 || maxprocs > reslimit.rlim_cur) maxprocs = reslimit.rlim_cur;

getrlimit realistically should never fail, so a failure with err(1, "getrlimit"); should be fine enough for surprising cases.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

sys/amd64/amd64/uma_machdep.c

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

Why? Other uses of strtou*() in the file or across the base don't use macros for integer bases.

usr.bin/xargs/xargs.c:                  Rflag = strtol(optarg, &endptr, 10);
usr.bin/xargs/xargs.c:                  Sflag = strtoul(optarg, &endptr, 10);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

Why allow value larger than RLIMIT_NPROC or _SC_CHILD_MAX? Soft limit would cap number of processes sooner. And kern.maxproc can be larger than "infinity" hard limit.

$ limits -Su 50 sh -c "yes | xargs -n1 -P0 yes foo" >/dev/null &
$ pgrep -f foo | wc -l
       6

$ limits -Su
Resource limits (current):
  maxprocesses-cur          100
$ limits -Hu
Resource limits (current):
  maxprocesses-max         1458
$ sysctl kern.max{proc,users}
kern.maxproc: 1620 # (20 + 16 * maxusers)
kern.maxusers: 100
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

I do not see how nargs is the correct value to use here; it has a different meaning to the program, and can be overridden by other command-line arguments, giving a bizzare behavior change if -n 1 -P 0 are passed in different orders.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

The code form user perspective should behave just like gnu xargs, unless there is a good reason to deviate, which I don't see.

In this particular case, they have "unlimited" for an argument == 0, specified limit for > 0 and error out for non-numbers and negative values. Here negative values are accepted for some reason.

One big not-user-visible difference is the fact that gnu xargs dynamically resizes the process table, while our xargs allocates it upfront. This means we have to have a reasonable limit which can pose as "unlimited" for practical purposes.

getrlimit seen here is fine enough.

However, the code must not complain about passed limit being bigger. It should internally cap it to getrlimit so that user-visible behaviour is identical with gnu in this respect.

As a result the code should explicitly coompare maxprocs to 0 (as opposed to < 1), proclim variable has no use. Fetch rlimit /after/ maxprocs is validated. If (maxprocs == 0 || maxprocs > reslimit.rlim_cur) maxprocs = reslimit.rlim_cur;

getrlimit realistically should never fail, so a failure with err(1, "getrlimit"); should be fine enough for surprising cases.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

sys/arm/arm/pmap-v6.c

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

Why? Other uses of strtou*() in the file or across the base don't use macros for integer bases.

usr.bin/xargs/xargs.c:                  Rflag = strtol(optarg, &endptr, 10);
usr.bin/xargs/xargs.c:                  Sflag = strtoul(optarg, &endptr, 10);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

Why allow value larger than RLIMIT_NPROC or _SC_CHILD_MAX? Soft limit would cap number of processes sooner. And kern.maxproc can be larger than "infinity" hard limit.

$ limits -Su 50 sh -c "yes | xargs -n1 -P0 yes foo" >/dev/null &
$ pgrep -f foo | wc -l
       6

$ limits -Su
Resource limits (current):
  maxprocesses-cur          100
$ limits -Hu
Resource limits (current):
  maxprocesses-max         1458
$ sysctl kern.max{proc,users}
kern.maxproc: 1620 # (20 + 16 * maxusers)
kern.maxusers: 100
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

I do not see how nargs is the correct value to use here; it has a different meaning to the program, and can be overridden by other command-line arguments, giving a bizzare behavior change if -n 1 -P 0 are passed in different orders.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

The code form user perspective should behave just like gnu xargs, unless there is a good reason to deviate, which I don't see.

In this particular case, they have "unlimited" for an argument == 0, specified limit for > 0 and error out for non-numbers and negative values. Here negative values are accepted for some reason.

One big not-user-visible difference is the fact that gnu xargs dynamically resizes the process table, while our xargs allocates it upfront. This means we have to have a reasonable limit which can pose as "unlimited" for practical purposes.

getrlimit seen here is fine enough.

However, the code must not complain about passed limit being bigger. It should internally cap it to getrlimit so that user-visible behaviour is identical with gnu in this respect.

As a result the code should explicitly coompare maxprocs to 0 (as opposed to < 1), proclim variable has no use. Fetch rlimit /after/ maxprocs is validated. If (maxprocs == 0 || maxprocs > reslimit.rlim_cur) maxprocs = reslimit.rlim_cur;

getrlimit realistically should never fail, so a failure with err(1, "getrlimit"); should be fine enough for surprising cases.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

sys/arm64/arm64/pmap.c

Loading...
View Options

sys/arm64/arm64/uma_machdep.c

Loading...
View Options

sys/arm64/iommu/iommu_pmap.c

Loading...
View Options

sys/compat/linuxkpi/common/src/linux_page.c

Loading...
View Options

sys/dev/drm2/ttm/ttm_page_alloc.c

Loading...
View Options

sys/dev/ti/if_ti.c

Loading...
View Options

sys/dev/virtio/balloon/virtio_balloon.c

Loading...
View Options

sys/dev/xen/balloon/balloon.c

Loading...
View Options

sys/dev/xen/gntdev/gntdev.c

Loading...
View Options

sys/fs/nfs/nfs_commonsubs.c

Loading...
View Options

sys/fs/nfsclient/nfs_clrpcops.c

Loading...
View Options

sys/i386/i386/pmap.c

Loading...
View Options

sys/kern/kern_mbuf.c

Loading...
View Options

sys/kern/uipc_ktls.c

Loading...
View Options

sys/kern/uipc_mbuf.c

Loading...
View Options

sys/kern/vfs_bio.c

Loading...
View Options

sys/mips/mips/pmap.c

Loading...
View Options

sys/mips/mips/uma_machdep.c

Loading...
View Options

sys/powerpc/aim/mmu_oea64.c

Loading...
View Options

sys/powerpc/aim/mmu_radix.c

Loading...
View Options

sys/powerpc/booke/pmap_32.c

Loading...
View Options

sys/powerpc/booke/pmap_64.c

Loading...
View Options

sys/powerpc/powerpc/uma_machdep.c

Loading...
View Options

sys/riscv/riscv/pmap.c

Loading...
View Options

sys/riscv/riscv/uma_machdep.c

Loading...
View Options

sys/vm/uma_core.c

Loading...
View Options

sys/vm/vm_kern.c

Loading...
View Options

sys/vm/vm_page.c

Loading...