Messages in this thread | | | From | (Eric W. Biederman) | Date | Wed, 17 May 2017 14:48:47 -0500 | Subject | Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself |
| |
Al Viro <viro@ZenIV.linux.org.uk> writes:
> On Mon, May 15, 2017 at 04:06:49PM -0700, Linus Torvalds wrote: >> On Mon, May 15, 2017 at 3:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: >> > >> > +struct waitid_info { >> > + pid_t pid; >> > + uid_t uid; >> > + int status; >> > + int why; >> > +}; >> >> Ugh. Could we please just name those with what they are actually used for? >> >> Even if you hate the "si_" previx for some reason, I really don't see >> why we'd continue call it "why", when it's written to "si_code" >> >> Yes, yes, I see the historical reason, and how "si_code" is just the >> low 16 bits of "why", and the high 16 bits is something else. > > __SI_CHLD, and AFAICS it only matters for copy_siginfo_to_user() and its > relatives - basically, "how much of kernel-side struct siginfo do we have > initialized"... > >> But now that there is a structure for that, could we not just make >> that explicit in the structure instead? Those games with "why" look >> really odd. > > OK... > >> So I can see why you'd like to keep this patch as "minimal >> conversion", but it would be really nice to have a followup patch that >> gets rid of the odd "why" games. > > The thing is, we lack convenient defines for those constants. We could > turn this "why" thing into u16 si_code, but then gcc will scream about > integer constant truncation ;-/ Suggestions?
Hmm. Given the small size of that thing I think I would just include embed waidtid_info in wait_opts as you have done earlier with wo_stat;
Which would mean at the end of do_wait you can just do:
wo->wo_info.si_code &= 0xffff;
Neither SI_TIMER nor SI_MESGQ are being used so sign extension is not needed.
As for getting magic out of the upper bits of si_code I suspect we can just switch on si_signo and then for the realtime signals si_code to get the layout in copy_siginfo_to_user.
The compiler just uses a binary tree of jumps so I don't expect the code generated for copy_signinfo_to_user would be much worse and I do expect with the magic taken out the logic of the code would be easier to understand.
Alternately we could use some kind of ksiginfo that does not take up 128 bytes (except in the case of rt_sigqueueinfo) and being kernel internal only has a format that is easy to decode for copy_siginfo_to_user.
> BTW, I wonder if making those stores conditional is actually a win - > sure, for put_user() it used to be, but for plain stores... Not sure.
My guess would be that storing to a small structure on the stack would be almost free, and with all of the fields unconditionally present in struct wait_opts the code could compute the values directly into wo without needing intermediate varriables. Which ought to make the code easier to understand and maintain if not cycle by cycle faster.
Eric
| |