| Message ID | 20201110111137.16117-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | um: Fetch registers only for signals which need them | expand |
On Tue, 2020-11-10 at 11:11 +0000, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > UML userspace fetches siginfo and passes it to signal handlers > in UML. This is needed only for some of the signals, because > key handlers like SIGIO make no use of this variable. It looks like even for SIGSEGV it depends on PTRACE_FULL_FAULTINFO, which is only set for 64-bit? > - ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); > + switch (sig) { > + case SIGSEGV: > + case SIGTRAP: > + case SIGILL: > + case SIGBUS: > + case SIGFPE: > + case SIGWINCH: > + ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); > + break; > + } > > switch (sig) { > case SIGSEGV: So perhaps it might make sense to push it further down into that switch? It'd end up duplicating it to three places though. In the SIGIO case you could write it as case SIGILL: case SIGBUS: case SIGFPE: case SIGWINCH: ptrace(...); fallthrough; case SIGIO: block_signals_trace(); // etc But I have no strong opinion either way really. I do think a comment is needed though, because we _do_ pass the pointer to the sigio handler ... or maybe separate that out just like handle_trap() etc.? johannes
On 10/11/2020 11:27, Johannes Berg wrote: > On Tue, 2020-11-10 at 11:11 +0000, anton.ivanov@cambridgegreys.com > wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> UML userspace fetches siginfo and passes it to signal handlers >> in UML. This is needed only for some of the signals, because >> key handlers like SIGIO make no use of this variable. > > It looks like even for SIGSEGV it depends on PTRACE_FULL_FAULTINFO, > which is only set for 64-bit? > > >> - ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); >> + switch (sig) { >> + case SIGSEGV: >> + case SIGTRAP: >> + case SIGILL: >> + case SIGBUS: >> + case SIGFPE: >> + case SIGWINCH: >> + ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); >> + break; >> + } >> >> switch (sig) { >> case SIGSEGV: > > So perhaps it might make sense to push it further down into that switch? > It'd end up duplicating it to three places though. > > In the SIGIO case you could write it as This is predominantly to save some CPU cycles for SIGALRM and SIGIO. The other ones are not invoked that often. The difference when doing time find /usr -type f -exec cat {} > /dev/null \; is ~ 1.5%, there is a similar gain in networking on iperf. > > case SIGILL: > case SIGBUS: > case SIGFPE: > case SIGWINCH: > ptrace(...); > fallthrough; > case SIGIO: > block_signals_trace(); > // etc > > But I have no strong opinion either way really. > > > I do think a comment is needed though, because we _do_ pass the pointer > to the sigio handler ... or maybe separate that out just like > handle_trap() etc.? Ack. > > johannes > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On 10/11/2020 11:27, Johannes Berg wrote: > On Tue, 2020-11-10 at 11:11 +0000, anton.ivanov@cambridgegreys.com > wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> UML userspace fetches siginfo and passes it to signal handlers >> in UML. This is needed only for some of the signals, because >> key handlers like SIGIO make no use of this variable. > > It looks like even for SIGSEGV it depends on PTRACE_FULL_FAULTINFO, > which is only set for 64-bit? > > >> - ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); >> + switch (sig) { >> + case SIGSEGV: >> + case SIGTRAP: >> + case SIGILL: >> + case SIGBUS: >> + case SIGFPE: >> + case SIGWINCH: >> + ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); >> + break; >> + } >> >> switch (sig) { >> case SIGSEGV: > > So perhaps it might make sense to push it further down into that switch? > It'd end up duplicating it to three places though. > > In the SIGIO case you could write it as > > case SIGILL: > case SIGBUS: > case SIGFPE: > case SIGWINCH: > ptrace(...); > fallthrough; gcc now complains loudly about fallthrough cases. Though I can override it, I'd rather leave the original approach where there is a list of SIGs which need the si fetched. > case SIGIO: > block_signals_trace(); > // etc > > But I have no strong opinion either way really. > > > I do think a comment is needed though, because we _do_ pass the pointer I added a comment to v2 which explains what and why is being done. > to the sigio handler ... or maybe separate that out just like > handle_trap() etc.? > > johannes > >
diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c index 4fb877b99dde..8123b1c6b1b5 100644 --- a/arch/um/os-Linux/skas/process.c +++ b/arch/um/os-Linux/skas/process.c @@ -400,7 +400,16 @@ void userspace(struct uml_pt_regs *regs, unsigned long *aux_fp_regs) if (WIFSTOPPED(status)) { int sig = WSTOPSIG(status); - ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); + switch (sig) { + case SIGSEGV: + case SIGTRAP: + case SIGILL: + case SIGBUS: + case SIGFPE: + case SIGWINCH: + ptrace(PTRACE_GETSIGINFO, pid, 0, (struct siginfo *)&si); + break; + } switch (sig) { case SIGSEGV: