Date: Mon, 4 Mar 2019 15:12:11 +0000 From: Edward Napierala <trasz@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: 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: <CAFLM3-pFFK_zcc5=5XPvi5n7RCT-GiNTa7To9VnPkwTp2HstTA@mail.gmail.com> In-Reply-To: <20190304143021.GO68879@kib.kiev.ua> References: <201903041302.x24D2aG0093620@repo.freebsd.org> <20190304132021.GN68879@kib.kiev.ua> <CAFLM3-pLSQ8sBawC9YBTgxdMKhtNtoQG1bn2QVDuw-2tDKb4Gg@mail.gmail.com> <20190304143021.GO68879@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
pon., 4 mar 2019 o 14:30 Konstantin Belousov <kostikbel@gmail.com> napisa= =C5=82(a): > > 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> napi= sa=C5=82(a): > > > > + p =3D curthread; > > > Why do you name it 'p', which is typical for process, and not 'td', y= ou are > > > changing most of the code anyway. > > > > To keep the diff size smaller. You're right, this touches a lot of stu= ff, > > 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 remo= ve 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 u= sed > > > as a formal placeholder argument only. But when the first action in t= he > > > 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 !=3D 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFLM3-pFFK_zcc5=5XPvi5n7RCT-GiNTa7To9VnPkwTp2HstTA>