Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 24 Jul 2018 16:24:28 -0500 | Subject | Re: [PATCH v2 20/20] signal: Don't restart fork when signals come in. |
| |
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Tue, Jul 24, 2018 at 1:40 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> + if (signal_pending(current)) { >> + retval = restart_syscall(); >> + goto fork_out; >> + } > > Oh, the previous version had this too, but it wasn't as obvious > because it was just in a single line: > > return ERR_PTR(restart_syscall()); > > but it's just crazy. > > It should just be > > retval = -ERESTARTNOINTR; > if (signal_pending(current)) > goto fork_out; > > because it's just silly and pointless to change the code to use > restart_syscall() here. > > All restart_syscall() does is > > set_tsk_thread_flag(current, TIF_SIGPENDING); > return -ERESTARTNOINTR; > > and you just *checked* that TIF_SIGPENDING was already set. So the > above is completely pointless. > > It is not clear why you made that change. The old code had the simpler > "just return -ERESTARTNOINTR" model. > > Did the restart_syscall() thing come in by mistake from some previous > trials and it just hung around?
I think this is the one place in the kernel where we can restart a system call and not set TIF_SIGPENDING.
Several years ago I made the mistake in the networking code of returning -ERESTARTNOINTR and forgetting to set TIF_SIGPENDING. That wasn't fun. So I wrote restart_syscall and use it so I don't make that mistake again.
In this case your suggesting will definitely generate better code so I am happy to make a V3 with that doesn't use restart_syscall. A person working in the guts of fork can reasonably be expected understand and to have all of the subtleties in cache as they work on that part of fork.
Eric
| |