Add fork_func #487

gperciva · 2023-06-26T16:50:54Z

No description provided.

gperciva · 2023-06-28T18:25:09Z

There's more overhead in fork_func(); fork_func_wait(); than in pthread, but it's less than 1ms, and more importantly, there isn't a lot of variability.

Tests when waiting for 0ms, 1ms, 30ms, and 90ms on FreeBSD 12.4 with kern.timecounter.alloweddeviation=0:

gperciva · 2023-06-28T18:26:21Z

This PR includes #488.

gperciva · 2023-06-28T18:37:39Z

Having a separate file check_exit.c seems like overkill. However, I wanted to have check_perftest.c and check_order.c in separate files. Once that was done, I could either have the separate check_exit.c, or add a check.c which only contained the exit-checking-related functions.

gperciva · 2023-06-28T18:39:52Z

Testing with valgrind on macOS does require the sleep-sort values to be separated by more than 100ms. Empirically, when tested with 300ms and 400ms, the order was sometimes reversed (1 out of 2 attempts; I didn't try more tests to get a statistical estimate).

I went with 200ms, because occasionally-failing tests are really annoying.

cperciva · 2023-08-26T22:44:08Z

util/fork_func.c

+{
+	pid_t pid;
+
+	/* Fork */


This is traditionally written with switch (pid = fork()), see fork(2) for example.

sigh, ok, I'll change it. I still think it's easier to leverage our normal "do something; did it fail?" idiom. Then if it did not fail, then figure out what type of success it had.

BTW, although I've seen the traditional switch method before, it's not in the freebsd 12.4 man page, which has no EXAMPLES section:
https://man.freebsd.org/cgi/man.cgi?query=fork&apropos=0&sektion=2&manpath=FreeBSD+12.4-RELEASE

Evidently that was only added in 13.1:
https://reviews.freebsd.org/D27626

cperciva · 2023-08-26T22:46:06Z

util/fork_func.c

+			warn0("pid %jd: stopped with %i",
+			    (intmax_t)pid, WSTOPSIG(status));
+		else
+			warn0("pid %jd: stopped for unknown reason.");


Is "stopped" what you want here? I mean, stopped != exited.

Good catch, changed.

gperciva · 2023-08-27T17:00:57Z

Updated with a REBASE commit.

I also rebased onto master, to clear up the git conflict.

cperciva · 2023-09-25T18:33:00Z

tests/fork_func/check_exec.c

+
+	(void)cookie; /* UNUSED */
+
+	if (execlp("true", "", NULL))


Is there a reason you want to invoke true(1) with an argv0 of ""?

nope, fixed.

cperciva · 2023-09-25T18:34:47Z

tests/fork_func/check_exit.c

+static int
+func_exit(void * cookie)
+{
+	int exitcode = *((int *)cookie);


I would write this as

int *exitcodep = (int *)cookie; return (*exitcodep);

Does the same thing, but we generally follow a convention of "pointer = cookie" at the start of callback functions.

cperciva · 2023-09-25T18:42:05Z

util/fork_func.c

+		if (WIFSIGNALED(status))
+			warn0("pid %jd: signalled with %i",
+			    (intmax_t)pid, WTERMSIG(status));
+		else if (WIFSTOPPED(status))


Do we care about reporting processes which were stopped but not terminated?

I figured that anything other than WIFEXITED was unusual and worth reporting?

I mean, the overall point of fork_func_wait() is "wait until the forked process is finished". I don't have a good sense of what to do if there's an error, so I figured that reporting it was a decent lower bound?

WIFSTOPPED(status) True if the process has not terminated, but has stopped and can be restarted. This macro can be true only if the wait call specified the WUNTRACED option or if the child process is being traced (see ptrace(2)).

Since we're not passing the WUNTRACED flag (or any flags at all for that matter), the only thing this would report to us is if the process is being traced... in which case the signal value isn't particularly useful anyway. So I would just handle this with the 'unknown reason' message.

ok, removed

cperciva · 2023-09-25T18:44:25Z

util/fork_func.c

+		goto err0;
+	case 0:
+		/* In child process: Run the provided function, then exit. */
+		_exit((*func)(cookie));


We generally omit the * when calling via a function pointer. (The C standard says it's optional.)

gperciva · 2023-09-25T19:01:43Z

util/fork_func.c

@@ -12,7 +12,7 @@
 * Fork and run ${func} in a new process, with ${cookie} as the sole argument.
 */
 pid_t
-fork_func(int (* func)(void *), void * cookie)
+fork_func(int (*func)(void *), void * cookie)


wait, now I'm rethinking this one.

in events/*.c, there's 6 function pointers defined as (*func) with no spaces. There's only 2 instances of (* func), both of which in cpusupport code I wrote.

OTOH, when I grep for (* , there's 56 hits, and at first glance, at least 90% of them are function pointers.

So I'm guessing that the desired style is:

int foo(int (* func)(void *))

?

Yes, our STYLE says "space between * and variable name". Applies to function pointers just as much as any other pointers.

I removed the bad commit, and made a separate PR for fixing the style of our function pointers.

gperciva · 2024-01-13T02:21:12Z

I rebased the existing REBASE commits, but then added one more small change.

cperciva · 2024-01-14T00:36:43Z

util/fork_func.c

+	/* Fork */
+	switch (pid = fork()) {
+	case -1:
+		warnp("fork");


For consistency with the other cases, I would add /* Error in fork system call. */

cperciva · 2024-01-14T00:37:22Z

util/fork_func.c

+	} else {
+		/* Child ${pid} did not exit cleanly. */
+
+		/* Warn about the reason for the unclean exit. */


Combine these into a single (multi-line) comment.

cperciva · 2024-01-14T00:39:21Z

util/fork_func.c

+
+		/* Warn about the reason for the unclean exit. */
+		if (WIFSIGNALED(status))
+			warn0("pid %jd: signalled with %i",


Maybe "terminated by signal" would be clearer?

Also, any reason you used %i instead of the more common %d?

Hold-over from when I was a TA: "use %i for integer, and %f for float". (Because to a first-year engineering student, "decimal" means something like "1.23".)

Ok, I'll switch to %d.

cperciva · 2024-01-14T00:40:35Z

A few minor nits but generally looks good. Feel free to make changes and rebase unless there's something you're not sure about.

gperciva · 2024-01-14T02:17:56Z

Fixes made, rebased, ready for merge.

gperciva closed this Jun 26, 2023

gperciva reopened this Jun 27, 2023

gperciva force-pushed the fork-func branch from fdc6edb to dbdffbb Compare June 27, 2023 21:08

gperciva closed this Jun 27, 2023

gperciva reopened this Jun 27, 2023

gperciva force-pushed the fork-func branch from dbdffbb to f4a0370 Compare June 27, 2023 21:19

gperciva marked this pull request as draft June 27, 2023 21:22

gperciva force-pushed the fork-func branch 4 times, most recently from e9ba002 to 9dfd1de Compare June 28, 2023 16:59

gperciva marked this pull request as ready for review June 28, 2023 17:02

gperciva marked this pull request as draft June 28, 2023 18:28

gperciva force-pushed the fork-func branch 2 times, most recently from 295e2de to 04a8cb3 Compare June 28, 2023 18:36

gperciva force-pushed the fork-func branch from 04a8cb3 to f9a5a36 Compare June 28, 2023 18:41

gperciva marked this pull request as ready for review June 30, 2023 00:23

gperciva force-pushed the fork-func branch from f9a5a36 to 6490132 Compare June 30, 2023 00:30

gperciva force-pushed the fork-func branch from 6490132 to b4f0b6d Compare August 7, 2023 00:25

gperciva force-pushed the fork-func branch from b4f0b6d to 34a49a7 Compare August 16, 2023 20:37

cperciva reviewed Aug 26, 2023

View reviewed changes

gperciva force-pushed the fork-func branch 2 times, most recently from ece2651 to 469d76f Compare August 27, 2023 17:00

gperciva force-pushed the fork-func branch from 8a394a1 to be0cfe4 Compare September 13, 2023 18:30

gperciva force-pushed the fork-func branch from be0cfe4 to ae5d375 Compare September 13, 2023 18:33

gperciva force-pushed the fork-func branch from ae5d375 to fe7d449 Compare September 21, 2023 02:58

gperciva marked this pull request as draft September 22, 2023 03:23

cperciva reviewed Sep 25, 2023

View reviewed changes

gperciva commented Sep 25, 2023

View reviewed changes

gperciva force-pushed the fork-func branch from cca45d5 to 976c5bf Compare September 26, 2023 02:14

gperciva force-pushed the fork-func branch from 976c5bf to c56174a Compare January 13, 2024 02:20

gperciva force-pushed the fork-func branch from c56174a to 4a8cd0b Compare January 13, 2024 02:30

cperciva reviewed Jan 14, 2024

View reviewed changes

gperciva added 4 commits January 13, 2024 19:15

Add fork_func.{c,h}

ebfe5c8

tests: add fork_func

19efda2

tests: add perftest for fork_func

7a8aa0f

build: run make Makefiles

5fe2b4f

gperciva force-pushed the fork-func branch from 4a8cd0b to 5fe2b4f Compare January 14, 2024 02:17

gperciva marked this pull request as ready for review January 14, 2024 02:17

cperciva merged commit 4b2eb56 into master Jan 14, 2024
2 checks passed

gperciva deleted the fork-func branch January 14, 2024 21:23

gperciva mentioned this pull request Jan 26, 2024

In printf, use %d instead of %i #510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fork_func #487

Add fork_func #487

gperciva commented Jun 26, 2023

gperciva commented Jun 28, 2023

gperciva commented Jun 28, 2023

gperciva commented Jun 28, 2023

gperciva commented Jun 28, 2023

cperciva Aug 26, 2023

gperciva Aug 27, 2023

cperciva Aug 26, 2023

gperciva Aug 27, 2023

gperciva commented Aug 27, 2023

cperciva Sep 25, 2023

gperciva Sep 25, 2023

cperciva Sep 25, 2023

gperciva Sep 25, 2023

cperciva Sep 25, 2023

gperciva Sep 25, 2023

cperciva Sep 25, 2023

gperciva Sep 26, 2023

cperciva Sep 25, 2023

gperciva Sep 25, 2023

gperciva Sep 25, 2023

cperciva Sep 26, 2023

gperciva Sep 26, 2023

gperciva commented Jan 13, 2024

cperciva Jan 14, 2024

cperciva Jan 14, 2024

cperciva Jan 14, 2024

gperciva Jan 14, 2024

cperciva commented Jan 14, 2024

gperciva commented Jan 14, 2024

Add fork_func #487

Add fork_func #487

Conversation

gperciva commented Jun 26, 2023

gperciva commented Jun 28, 2023

gperciva commented Jun 28, 2023

gperciva commented Jun 28, 2023

gperciva commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gperciva commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gperciva commented Jan 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cperciva commented Jan 14, 2024

gperciva commented Jan 14, 2024