Date: Mon, 4 Mar 2019 23:13:05 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Edward Napierala <trasz@freebsd.org>, Cy Schubert <Cy.Schubert@cschubert.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, src-committers <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver Message-ID: <QB1PR01MB3537715B615DC86781069252DD710@QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <201903042025.x24KP9uU021607@slippy.cwsent.com> References: Message from Edward Napierala <trasz@freebsd.org> of "Mon, 04 Mar 2019 15:26:39 %2B0000." <CAFLM3-pFi_-yhMx4bhm_0S5FK3BdLh1xbXV%2BcKgPks7d8cttMw@mail.gmail.com>, <201903042025.x24KP9uU021607@slippy.cwsent.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Cy Schubert wrote: >Sent: Monday, March 4, 2019 3:25 PM >To: Edward Napierala >Cc: Cy Schubert; Konstantin Belousov; src-committers; svn-src-all@freebsd.= org; svn-src->head@freebsd.org >Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver > >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= =C5=82(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=C5=82(a): >> > >> > > + p =3D 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 dow= n >> > >> 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 passi= ng >> > > (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. >> > > >> > >Before you start doing a lot of small changes (AKA continous churn) >> > >please formulate your goals and get some public feedback. My immedia= te >> > >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 b= e 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, plea= se? >> >> The review from the NFS maintainer is not enough? >> Btw, I've never listed myself as the NFS maintainer. I need to go look to s= ee if someone else put me in the maintainer's list. I understand that it is mostl= y authored by me, but I consider it FreeBSD project code once committed. (Others do co= mmits to it without my review and that is just fine with me.) >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. This is way too technical for me. I can barely look at a "diff" and make se= nse of it.;-) It sounds like there needs to be a discussion (on freebsd-fs@ perhaps?) of = the "big picture change" here. All I am going to do with the patches in phabricator is take a quick look t= o see if I can spot anything that will break the code. (I did mention that I didn't understand why he was doing this in one of the= reviews, but noted that it didn't break anything.) Oh, and the variable was called "p" because the code started in OpenBSD, wh= ere it was a proc ptr and I kept it portable by replacing "struct proc" with NFSPR= OC_T. (This portability is no longer a consideration.) I'll hold off on further phabricator reviews until the "big picture" change= gets discussed on some mailing list. (I don't see phabricator as the correct tool for "big= picture" discussions.) rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?QB1PR01MB3537715B615DC86781069252DD710>