Date: Thu, 23 Feb 2023 06:17:51 -0800 From: Rick Macklem <rick.macklem@gmail.com> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Rick Macklem <rmacklem@freebsd.org>, bz@freebsd.org, jamie@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: ef6fcc5e2b07 - main - nfsd: Add VNET_SYSUNINIT() macros for vnet cleanup Message-ID: <CAM5tNy5sNFB3_cNH8jFP8=3gvt1eLowukM43Zo98eB64kH1kYQ@mail.gmail.com> In-Reply-To: <Y/cHpcn/1S3Q0Swa@FreeBSD.org> References: <202302202112.31KLCfQB080359@gitrepo.freebsd.org> <Y/Uz5B6ywWjaa1TB@FreeBSD.org> <CAM5tNy6KFzCarKWze4mjbnqsERUoshHFfZeXzZMF3RurVKf7TA@mail.gmail.com> <Y/cHpcn/1S3Q0Swa@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Feb 22, 2023 at 10:28 PM Gleb Smirnoff <glebius@freebsd.org> wrote: > > Rick, > > On Tue, Feb 21, 2023 at 07:29:30PM -0800, Rick Macklem wrote: > R> I did that for one of the ones related to nfsd (sys/fs/nfsserver/nfs_fha_new.c). > > This one actually doesn't look correct to me. What happens here is that the sysctls > will affect only the default VNET. > I can pull the sysctls out of softc and vnet them. I will look at that to-day and should be easy to do. I'll admit I didn't bother, since they are basically of little use, only affect NFSv3 and, yes, I was being lazy. The sysctls in the krpc pool are another story. Not easy to move out of the pool, so I think they are going to be prison0 only for a long time. They probably are rarely used (I didn't code the krpc), so I think having them only on prison0 is ok. The ones people might set (that control how many nfsd threads are generated) can be set in the prisons using the command line options on nfsd(8), so the sysctls are not needed in this case. rick > I think the VNET-itezation of this file went a bit wrong. You don't need to convert > static structures to malloced when you VNET-ize a module. The infrastructure should > take care of memory management. > > The dynamic sysctl context seems to be unneeded from the very beginning. It was > always attached to the static softc. Suggested patch: > > https://reviews.freebsd.org/D38742 > > It gets rid of one case of IS_DEFAULT_VNET. > > Tested very little: run two jails and observed difference in sysctl. > > R> It so happens it doesn't work for this case. If you look at the patch, you'll > R> see that the variable is vnet'd. It happens that it is only malloc'd for prisons > R> other than prison0. (For prison0, it points to a global structure that is not > R> malloc'd and is shared with the NFS client, which is not vnet'd. > > I think all of this instances can be fixed. > > For example nfsstatsv1, which is now malloced for non-default vnets and static for > the default. Lots of modules still incorrectly update the global one. > > This patch makes it right: > > https://reviews.freebsd.org/D38743 > > Tested very little: run two jails, use some NFS, observe difference in nfsstat. > > Removes another IS_DEFAULT_VNET. > > Note that D38743 is still far from perfect. NFS would benefit a lot if used macros > for per-VNET per-CPU (raceless!) stats, that are used in TCP/IP. Sorry, they aren't > documented :( You can find them in vnet.h starting from VNET_PCPUSTAT_DECLARE() > and look for example usage in TCP/IP. > > Please fill free to commandeer those reviews from me and finalize them if you > agree with general idea. > > -- > Gleb Smirnoff
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM5tNy5sNFB3_cNH8jFP8=3gvt1eLowukM43Zo98eB64kH1kYQ>