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

[Edward Napierala]

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 reviewed 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?