From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752796AbdEQTz1 (ORCPT ); Wed, 17 May 2017 15:55:27 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:49603 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750795AbdEQTzY (ORCPT ); Wed, 17 May 2017 15:55:24 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Al Viro Cc: Linus Torvalds , Linux Kernel Mailing List , Ingo Molnar , Oleg Nesterov , Peter Zijlstra , Christoph Hellwig References: <20170515223157.GM390@ZenIV.linux.org.uk> <20170515223716.2085-1-viro@ZenIV.linux.org.uk> <20170515223716.2085-4-viro@ZenIV.linux.org.uk> <20170515234633.GN390@ZenIV.linux.org.uk> Date: Wed, 17 May 2017 14:48:47 -0500 In-Reply-To: <20170515234633.GN390@ZenIV.linux.org.uk> (Al Viro's message of "Tue, 16 May 2017 00:46:33 +0100") Message-ID: <87tw4j9sdc.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dB52Q-0001h6-TT;;;mid=<87tw4j9sdc.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=97.121.81.159;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/IS8p0v3pIsytXMUWJn+jl4Dr/50dIOZo= X-SA-Exim-Connect-IP: 97.121.81.159 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.7 XMSubLong Long Subject * 1.5 TR_Symld_Words too many words that have symbols inside * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_02 5+ unique symbols in subject * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ***;Al Viro X-Spam-Relay-Country: X-Spam-Timing: total 5300 ms - load_scoreonly_sql: 0.03 (0.0%), signal_user_changed: 2.3 (0.0%), b_tie_ro: 1.60 (0.0%), parse: 0.74 (0.0%), extract_message_metadata: 12 (0.2%), get_uri_detail_list: 1.94 (0.0%), tests_pri_-1000: 6 (0.1%), tests_pri_-950: 1.16 (0.0%), tests_pri_-900: 0.96 (0.0%), tests_pri_-400: 23 (0.4%), check_bayes: 22 (0.4%), b_tokenize: 7 (0.1%), b_tok_get_all: 8 (0.1%), b_comp_prob: 2.4 (0.0%), b_tok_touch_all: 2.5 (0.0%), b_finish: 0.52 (0.0%), tests_pri_0: 239 (4.5%), check_dkim_signature: 0.56 (0.0%), check_dkim_adsp: 3.1 (0.1%), tests_pri_500: 5014 (94.6%), poll_dns_idle: 5008 (94.5%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Al Viro 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 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