Date: Tue, 5 Mar 2019 18:18:17 +0000 From: Edward Napierala <trasz@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: FreeBSD FS <freebsd-fs@freebsd.org> Subject: Re: 'td' vs sys/fs/nfsserver/ Message-ID: <CAFLM3-qRmRcjYvxzboLuR2FZDW%2B_YgiBxR2oxqYM0hXPA9w6ag@mail.gmail.com> In-Reply-To: <20190305153240.GB2492@kib.kiev.ua> References: <CAFLM3-ouStEoEmXUmgJzaR5RoR954a4-RdK%2BNe6dzzqzsr50-A@mail.gmail.com> <20190305153240.GB2492@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
wt., 5 mar 2019 o 15:33 Konstantin Belousov <kostikbel@gmail.com> napisa=C5= =82(a): > > On Tue, Mar 05, 2019 at 09:26:43AM +0000, Edward Napierala wrote: > > Hello. As many of you know, right now, throughout the kernel, > > there=E2=80=99s the =E2=80=98td=E2=80=99 argument being passed over and= over, even though > > we have a cheap way of accessing it by using =E2=80=98curthread=E2=80= =99. That=E2=80=99s > > suboptimal for obvious reasons. It can=E2=80=99t be fixed =E2=80=98jus= t like that=E2=80=99, > > as it would introduce a lot of code churn, and also possible bugs > > in case we miss a case where the =E2=80=98td=E2=80=99 is not equal to c= urthread. > > So, here's the big picture: this is what I'm _not_ intending > > to do at this point. > > > > What I do intend doing is to go and try to fix it in a single kernel > > component, the NFS server. The idea: drop the =E2=80=98td=E2=80=99 arg= ument (in > > case of NFS server it=E2=80=99s usually spelled =E2=80=98p=E2=80=99 due= to historical > > reasons) where it=E2=80=99s unused, which turns out to be quite often, = and > > otherwise push =E2=80=98td=E2=80=99 down, function by function, so that= when you > > review it it=E2=80=99s obvious that it=E2=80=99s equal to curthread. T= here are > > three reasons to do this: first, it makes it very obvious that > > the=E2=80=A8td passed to various VOPs it calls is indeed always curthre= ad, > > and makes it easier to do the change described in the previous > > paragraph, should someone try it. Second=E2=80=A6 well, it=E2=80=99s a= cleanup: > > the NFS code passes it everywhere, and in many cases it's not used > > at all. Third, and this is the man reason: it=E2=80=99s a good way to = test > > the idea of pushing td down layer by layer without touching any > > APIs that are not local to the NFS server, to see if it works, and > > if it doesn't annoy people to much. So, since I=E2=80=99ve been asked = to > > discuss it in public first: what do you guys think? > > Lets put aside the importance of the problem. > > I find the selection of the component to remove the 'td' argument somewha= t > strange. It is on top of the VFS stack, so any VOPs or other VFS functio= ns > calls taking 'td' would require insertion of many curthread()s. Why not > start from the bottom ? > > I believe that looking at all in-tree filesystems and deciding which VOPs > and VFS_XXX methods do not need td because they do not use it, would give > more useful approach. After td is elminated from VOPs which do not need > it, it should be rather uncontended to eleminate td from corresponding > callers of them. At the bottom of the call stack the thread pointer usually gets used... eventually. Yes, that means we might end up accessing curthread several times. But I'd expect it to be still much cheaper[1] to just use curthread there, at the bottom, instead of passing the td over five or ten levels of function calls to get there; not to mention more readable. Also, there's a common pattern of filesystem functions calling global functions, which in turn back into filesystem (eg ufs_accessx -> VOP_GETACL -> ufs_getacl -> vn_extattr_get -> VOP_GETEXTATTR etc), which means doing it step by step would introduce a lot of KPI changes, which is not something I'd like to do. Remember when we talked about it on IRC some... ten years ago? I'm not sure, but I think it was you. I've asked if the 'td' argument, I think it was about the td argument to VOPs in general, was always equal to curthread. And the answer was "probably". This means I can't just safely ignore it and use curthread when I need it. I mean, sure I can, but it might bite me in the end. So the safe way is to just keep passing it over and over again, everywhere, in newly written code. And thus the top-down approach. Doing cleanup at the top first, even without actually changing the public kernel KPIs, means the 'probably' turns into 'yes', and in the new code I can stop passing it over and over. 1. Where "much cheaper" means "it's unmeasurable anyway".
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFLM3-qRmRcjYvxzboLuR2FZDW%2B_YgiBxR2oxqYM0hXPA9w6ag>