svn commit: r344758 - in head/sys/fs: nfs nfsserver

Cy Schubert Cy.Schubert at cschubert.com
Mon Mar 4 20:25:35 UTC 2019


In message <CAFLM3-pFi_-yhMx4bhm_0S5FK3BdLh1xbXV+cKgPks7d8cttMw at mail.gma
il.com>
, Edward Napierala writes:
> pon., 4 mar 2019 o 15:17 Cy Schubert <Cy.Schubert at cschubert.com> napisał(a):
> >
> > On March 4, 2019 6:30:21 AM PST, Konstantin Belousov <kostikbel at 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 at 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 at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.




More information about the svn-src-all mailing list