Date: Mon, 04 Mar 2019 12:25:09 -0800 From: Cy Schubert <Cy.Schubert@cschubert.com> To: Edward Napierala <trasz@freebsd.org> Cc: Cy Schubert <Cy.Schubert@cschubert.com>, Konstantin Belousov <kostikbel@gmail.com>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver Message-ID: <201903042025.x24KP9uU021607@slippy.cwsent.com> In-Reply-To: Message from Edward Napierala <trasz@freebsd.org> of "Mon, 04 Mar 2019 15:26:39 %2B0000." <CAFLM3-pFi_-yhMx4bhm_0S5FK3BdLh1xbXV%2BcKgPks7d8cttMw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <CAFLM3-pFi_-yhMx4bhm_0S5FK3BdLh1xbXV+cKgPks7d8cttMw@mail.gma il.com> , Edward Napierala writes: > pon., 4 mar 2019 o 15:17 Cy Schubert <Cy.Schubert@cschubert.com> napisał(a): > > > > On March 4, 2019 6:30:21 AM PST, Konstantin Belousov <kostikbel@gmail.com> > wrote: > > >On Mon, Mar 04, 2019 at 01:31:37PM +0000, Edward Napierala wrote: > > >> pon., 4 mar 2019 o 13:20 Konstantin Belousov <kostikbel@gmail.com> > > >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 <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201903042025.x24KP9uU021607>