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

Edward Napierala trasz at freebsd.org
Mon Mar 4 15:12:24 UTC 2019


pon., 4 mar 2019 o 14:30 Konstantin Belousov <kostikbel at gmail.com> napisał(a):
>
> 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.

But this way I create less churn - if I renamed it to 'td', then first
I'd change
the calls to other functions to take 'td' instead of 'p', and then I'd remove
this argument altogether in subsequent commit.  This would make diffs
larger for no good reason.

> > > 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.

Ah, ok.  So, yes, you are right that from a high level point of view
it's a poor argument.  But at this point I'm not trying to change stuff
throughout the kernel; just the NFS server.  And the reason for doing
this is to make it obvious that the 'td' argument it passes to VOPs and
other kernel APIs is always equal to curthread.  Doing it layer by layer
makes it easy to review.

> Before you start doing a lot of small changes (AKA continous churn)
> please formulate your goals and get some public feedback.

I'll do that; I won't go changing kernel APIs like that.  But I'd like to
untangle things that complicate the picture, and the NFS server code
is one of them.

>  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)

Probably nothing; those things pretty much always actually use the thread
pointer.


More information about the svn-src-head mailing list