From owner-svn-src-all@freebsd.org Mon Mar 4 20:25:35 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 62FAA1521AEC; Mon, 4 Mar 2019 20:25:35 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from smtp-out-no.shaw.ca (smtp-out-no.shaw.ca [64.59.134.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id ECC5586469; Mon, 4 Mar 2019 20:25:33 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from spqr.komquats.com ([70.67.125.17]) by shaw.ca with ESMTPA id 0u9Rh4UtOEtQm0u9Th9WET; Mon, 04 Mar 2019 13:25:31 -0700 X-Authority-Analysis: v=2.3 cv=Vqpd/N+n c=1 sm=1 tr=0 a=VFtTW3WuZNDh6VkGe7fA3g==:117 a=VFtTW3WuZNDh6VkGe7fA3g==:17 a=8nJEP1OIZ-IA:10 a=NTGMnVQrEZIA:10 a=xfDLHkLGAAAA:8 a=YxBL1-UpAAAA:8 a=pGLkceISAAAA:8 a=6I5d2MoRAAAA:8 a=yyVpRMpY5pa3p9pvJCcA:9 a=7NTLRdJfDz6b4EGh:21 a=_VKl0IChOzxr_xR7:21 a=wPNLvfGTeEIA:10 a=UJ0tAi3fqDAA:10 a=IfaqVvZgccqrtc8gcwf2:22 a=Ia-lj3WSrqcvXOmTRaiG:22 a=IjZwj45LgO3ly-622nXo:22 Received: from slippy.cwsent.com (slippy8 [10.2.2.6]) by spqr.komquats.com (Postfix) with ESMTPS id EA230763; Mon, 4 Mar 2019 12:25:28 -0800 (PST) Received: from slippy.cwsent.com (localhost [127.0.0.1]) by slippy.cwsent.com (8.15.2/8.15.2) with ESMTP id x24KP93r021610; Mon, 4 Mar 2019 12:25:09 -0800 (PST) (envelope-from Cy.Schubert@cschubert.com) Received: from slippy (cy@localhost) by slippy.cwsent.com (8.15.2/8.15.2/Submit) with ESMTP id x24KP9uU021607; Mon, 4 Mar 2019 12:25:09 -0800 (PST) (envelope-from Cy.Schubert@cschubert.com) Message-Id: <201903042025.x24KP9uU021607@slippy.cwsent.com> X-Authentication-Warning: slippy.cwsent.com: cy owned process doing -bs X-Mailer: exmh version 2.8.0 04/21/2012 with nmh-1.7.1 Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Edward Napierala cc: Cy Schubert , Konstantin Belousov , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver In-Reply-To: Message from Edward Napierala of "Mon, 04 Mar 2019 15:26:39 +0000." Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Date: Mon, 04 Mar 2019 12:25:09 -0800 X-CMAE-Envelope: MS4wfM0ozJjBytrUdds4XIyLoByzbh6FMxedOygaYolECCDVZoetcAerAHVpXWu5RVpIfQYeeCPH2zonLjOV5qsqVZ+1tahy8ynmj80UqCt8BYiUsITRhZfU JZqCV3b4s9jRLKLRbJnxjmSNptfnGU6UEDGwCuGDMqReftwF7UArlrbi3BU2OHZ5gdWTTQdTzqFS6GqJprvUe8bAcBXR7lj1w+nOKHEcEEsFNL//k5iqy8wY HSndpQwA2aNH89RrvBfgjhnmhWcDuD28J7WCwgkArN+nTKQdyEx+cHVO5I8bQHOxSKo0EKiLvntyq1okCOJ9pw== X-Rspamd-Queue-Id: ECC5586469 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-4.88 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; RCVD_COUNT_FIVE(0.00)[5]; FROM_EQ_ENVFROM(0.00)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; MV_CASE(0.50)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[text/plain]; HAS_XAW(0.00)[]; HAS_REPLYTO(0.00)[Cy.Schubert@cschubert.com]; RCPT_COUNT_FIVE(0.00)[6]; REPLYTO_EQ_FROM(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MX_GOOD(-0.01)[cached: spqr.komquats.com]; NEURAL_HAM_SHORT(-0.97)[-0.973,0]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_SPF_NA(0.00)[]; RCVD_IN_DNSWL_LOW(-0.10)[12.134.59.64.list.dnswl.org : 127.0.5.1]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:6327, ipnet:64.59.128.0/20, country:CA]; RCVD_TLS_LAST(0.00)[]; IP_SCORE(-2.20)[ip: (-5.98), ipnet: 64.59.128.0/20(-2.76), asn: 6327(-2.17), country: CA(-0.09)]; RECEIVED_SPAMHAUS_PBL(0.00)[17.125.67.70.zen.spamhaus.org : 127.0.0.11] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Mar 2019 20:25:35 -0000 In message , Edward Napierala writes: > pon., 4 mar 2019 o 15:17 Cy Schubert napisał(a): > > > > On March 4, 2019 6:30:21 AM PST, Konstantin Belousov > wrote: > > >On Mon, Mar 04, 2019 at 01:31:37PM +0000, Edward Napierala wrote: > > >> pon., 4 mar 2019 o 13:20 Konstantin Belousov > > >napisał(a): > > >> > > + p = curthread; > > >> > Why do you name it 'p', which is typical for process, and not 'td', > > >you are > > >> > changing most of the code anyway. > > >> > > >> To keep the diff size smaller. You're right, this touches a lot of > > >stuff, > > >> but most of those added lines are temporary anyway - they will be > > >> removed later, when the td is pushed down even more. > > >But if you create code churn, doing it only half way is worse. > > > > > >> > > >> > Also I am curious why. It is certainly fine to remove td when it is > > >used > > >> > as a formal placeholder argument only. But when the first action in > > >the > > >> > function is evaluation of curthread() it becomes less obvious. > > >> > > >> Again, many/most of those are temporary. I'm trying to push td down > > >> in small steps, "layer by layer", so it's easy to review. > > >> > > >> > curthread() become very cheap on modern amd64, I am not so sure > > >about > > >> > older machines or non-x86 cases. > > >> > > >> The main reason is readability. Right now there's no easy way to > > >tell whether > > >> a function can be passed any td, or if it must be curthread. > > >I must admit that this is the weirdnest argument against 'td' that I > > >ever > > >heard. I saw more or less reasonable argumentation > > >- that using less arguments make one more register for argument passing > > > (amd64 has 6 input arg regs), > > >- that less arguments make smaller call code. > > >But trust me, in all cases where function can take td != curthread, it > > >is > > >either obvious or well-known for anybody who works with that code. > > > > > >Before you start doing a lot of small changes (AKA continous churn) > > >please formulate your goals and get some public feedback. My immediate > > >question that I want answered before you ever start touching the code, > > >is what you plan to do with > > > sys_syscall(struct thread *td, uap) > > > > Agreed on all points. At the very least this group of commits should be rev > iewed on phabricator. > > It has been, even though they are pretty much mechanical changes. > > > Can we back all these commits out until there is a proper review, please? > > The review from the NFS maintainer is not enough? > As it's NFS-only maybe though for anything substantial, like this, the more eyes the better. I'd agree with kib@ that we want to keep the amount of churn down, though it's understood that you want to separate the functional changes from the cosmetic ones. I tend to do the review and use git svn to separate the functional from the cosmetic changes, batching changes if I can. It's more work but IMO well worth it. -- Cheers, Cy Schubert FreeBSD UNIX: Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.