From owner-svn-src-all@freebsd.org Mon Mar 4 23:25:03 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 315CF1526DE2; Mon, 4 Mar 2019 23:25:03 +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 6E1598DFA2; Mon, 4 Mar 2019 23:25:02 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from spqr.komquats.com ([70.67.125.17]) by shaw.ca with ESMTPA id 0wx7hbNCeJjE40wx9hqVxB; Mon, 04 Mar 2019 16:25:00 -0700 X-Authority-Analysis: v=2.3 cv=Lq7sNUVc c=1 sm=1 tr=0 a=VFtTW3WuZNDh6VkGe7fA3g==:117 a=VFtTW3WuZNDh6VkGe7fA3g==:17 a=IkcTkHD0fZMA:10 a=NTGMnVQrEZIA:10 a=hF2rLc1pAAAA:8 a=6I5d2MoRAAAA:8 a=xfDLHkLGAAAA:8 a=YxBL1-UpAAAA:8 a=pGLkceISAAAA:8 a=CzpIpZ3pSPfZevzj2WcA:9 a=jMizxHKGjwZ4905R:21 a=Wtuxhp6T8zpQJ01D:21 a=QEXdDO2ut3YA:10 a=UJ0tAi3fqDAA:10 a=O9OM7dhJW_8Hj9EqqvKN:22 a=IjZwj45LgO3ly-622nXo:22 a=IfaqVvZgccqrtc8gcwf2:22 a=Ia-lj3WSrqcvXOmTRaiG:22 Received: from android-68f84e02b5988183.esitwifi.local (S0106788a207e2972.gv.shawcable.net [70.66.154.233]) by spqr.komquats.com (Postfix) with ESMTPSA id 7D835A29; Mon, 4 Mar 2019 15:24:56 -0800 (PST) Date: Mon, 04 Mar 2019 15:24:33 -0800 User-Agent: K-9 Mail for Android In-Reply-To: References: Message from Edward Napierala of "Mon, 04 Mar 2019 15:26:39 +0000." , <201903042025.x24KP9uU021607@slippy.cwsent.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver To: Rick Macklem ,Edward Napierala CC: Konstantin Belousov , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" From: Cy Schubert Message-ID: <59652042-485B-41DE-9421-DC7AA6CC8D83@cschubert.com> X-CMAE-Envelope: MS4wfMHENnoIH/li/HreDyO29NB0Y2H6bLCX2MhSYMYdO5rRv1lV2z+sQjhkucxiw2jpwgWIcsrZwasoP4z7hUaRjiCugG2r/EyZqqa1VnDQmKE4hFkO83wg fvy3FsT3AVhYLJLYHHry0bG0V7d7FoyBpxVfyVJgql1+noHBrpfcPwfEiXswwG6CCnq2bMl57JEOjoUdixiuhInpVFyf+sPuzH+AARMfsLHqU+ECRGtZgmDM zQCYrKW4SHxmdbWK1O5tT3DkW+A/hyXw5mR6YarKSz+/ld2gY4Ts2qT29IwICF/CRxhAX5oF9nvV9OZ4WrWtZakhz33Adboc5KxcU1+4G3x/BcS9JR47jm+s Nm0p/pYX X-Rspamd-Queue-Id: 6E1598DFA2 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.97 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.97)[-0.965,0] 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 23:25:03 -0000 On March 4, 2019 3:13:05 PM PST, Rick Macklem wrot= e: >Cy Schubert wrote: >>Sent: Monday, March 4, 2019 3:25 PM >>To: Edward Napierala >>Cc: Cy Schubert; Konstantin Belousov; src-committers; >svn-src-all@freebsd=2Eorg; svn-src->head@freebsd=2Eorg >>Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver >> >>In message >il=2Ecom> >, Edward Napierala writes: >>> pon=2E, 4 mar 2019 o 15:17 Cy Schubert >napisa=C3=85=E2=80=9A(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=2E, 4 mar 2019 o 13:20 Konstantin Belousov > >>> > >napisa=C3=85=E2=80=9A(a): >>> > >> > > + p =3D curthread; >>> > >> > Why do you name it 'p', which is typical for process, and not >'td', >>> > >you are >>> > >> > changing most of the code anyway=2E >>> > >> >>> > >> To keep the diff size smaller=2E 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=2E >>> > >But if you create code churn, doing it only half way is worse=2E >>> > > >>> > >> >>> > >> > Also I am curious why=2E It is certainly fine to remove td when >it is >>> > >used >>> > >> > as a formal placeholder argument only=2E But when the first >action in >>> > >the >>> > >> > function is evaluation of curthread() it becomes less >obvious=2E >>> > >> >>> > >> Again, many/most of those are temporary=2E I'm trying to push td >down >>> > >> in small steps, "layer by layer", so it's easy to review=2E >>> > >> >>> > >> > curthread() become very cheap on modern amd64, I am not so >sure >>> > >about >>> > >> > older machines or non-x86 cases=2E >>> > >> >>> > >> The main reason is readability=2E Right now there's no easy way >to >>> > >tell whether >>> > >> a function can be passed any td, or if it must be curthread=2E >>> > >I must admit that this is the weirdnest argument against 'td' >that I >>> > >ever >>> > >heard=2E 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=2E >>> > >But trust me, in all cases where function can take td !=3D >curthread, it >>> > >is >>> > >either obvious or well-known for anybody who works with that >code=2E >>> > > >>> > >Before you start doing a lot of small changes (AKA continous >churn) >>> > >please formulate your goals and get some public feedback=2E 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=2E At the very least this group of commits >should be rev >>> iewed on phabricator=2E >>> >>> It has been, even though they are pretty much mechanical changes=2E >>> >>> > Can we back all these commits out until there is a proper review, >please? >>> >>> The review from the NFS maintainer is not enough? >>> >Btw, I've never listed myself as the NFS maintainer=2E I need to go look >to see if >someone else put me in the maintainer's list=2E I understand that it is >mostly authored >by me, but I consider it FreeBSD project code once committed=2E (Others >do commits >to it without my review and that is just fine with me=2E) > >>As it's NFS-only maybe though for anything substantial, like this, the >>more eyes the better=2E >> >>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=2E I tend to do the review and use git svn to >>separate the functional from the cosmetic changes, batching changes if >>I can=2E It's more work but IMO well worth it=2E >This is way too technical for me=2E I can barely look at a "diff" and >make sense of it=2E;-) > >It sounds like there needs to be a discussion (on freebsd-fs@ perhaps?) >of the >"big picture change" here=2E > >All I am going to do with the patches in phabricator is take a quick >look to see >if I can spot anything that will break the code=2E >(I did mention that I didn't understand why he was doing this in one of >the reviews, > but noted that it didn't break anything=2E) IMO the why is always more important than the how=2E If there is no reason= why then how is irrelevant=2E > >Oh, and the variable was called "p" because the code started in >OpenBSD, where it >was a proc ptr and I kept it portable by replacing "struct proc" with >NFSPROC_T=2E >(This portability is no longer a consideration=2E) > >I'll hold off on further phabricator reviews until the "big picture" >change gets discussed >on some mailing list=2E (I don't see phabricator as the correct tool for >"big picture" >discussions=2E) > >rick Hopefully my inline reply on this phone worked=2E If not I'll try again to= night=2E --=20 Pardon the typos and autocorrect, small keyboard in use=2E Cheers, Cy Schubert FreeBSD UNIX: Web: http://www=2EFreeBSD=2Eorg The need of the many outweighs the greed of the few=2E