From nobody Thu Feb 23 13:56:06 2023 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4PMvh82n5mz3tlpv; Thu, 23 Feb 2023 13:56:24 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4PMvh80pc4z4JJ5; Thu, 23 Feb 2023 13:56:24 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pj1-x1030.google.com with SMTP id qi12-20020a17090b274c00b002341621377cso12850252pjb.2; Thu, 23 Feb 2023 05:56:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=2l9mJIKquX61zN/10GHvMPC9TH1nyV3mVEZdYHyvQVY=; b=TQXIZQ6cM7vkrAsaKWOzL6OAv1puSSB4vaWjcVtFlIFMtwxL5F6ZOOz2mPyI48Igio 6VafsjSuKUfk+NWCxUuW6bAmdyhP2il4HZRdKY3S7xygWKJJz3VY/L9oqHWfNyPGCNzt 94+f6IAAI7ncZ2YBy5KD1cLzCavt79tX37/1GBeUiROnADGEh4qMmE+q9ImyvgESlOiq 88Fa5SipKxcE5SnO1Mgnn3j5EpQCCV6dNQ0/Khx+atr9LEyDkTQoCkjXqeFL9/wx0IDz /F6G1faQNlLMCO9GJEIByAqe9SPFh9JdeshDpCOzDB2Prhu3/Zzee9i8d4kTM/DLLMev E9QA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=2l9mJIKquX61zN/10GHvMPC9TH1nyV3mVEZdYHyvQVY=; b=DXPVcXlv3EHm2W3YOk+8xIs4h88OgXTcWMFuG6mYMBfbzTofm5PSsnsRUOAYQ/zZ8G tUod14kp1uaF7GEtukbs7Ch2HAAR79ZkY1Pais2wdUtSYBR+9Sql7zfK/fgIePJtGqnW DYqUz63Zgw5HzBwcoKx2doVac8mxjmulh1tX2mv3/ITKybG0wyBTGmddRQhmBhl9bswS xaUthfcFfDS1hjTow/Y7BO7PYnq5UskoaIDQJT4QQsGCoo1T03wKRAWVGO9mdxVfWaMV fjzaHFgt466AHnr3HZuLFs9K5auPdAnwmc+DAnyowhJOcK8Hf66cCfnWQksLTb7nDfdm gpZQ== X-Gm-Message-State: AO0yUKXA74y1AE3W2y79t24AMoFvt5CTSe/Dud4u1gQDU64jDZFaUgyn IRyA1wVATuVo9Ahi3rrV+EjNWjRiCsifnmFK0lYiL2TBC8HS X-Google-Smtp-Source: AK7set/R9y1nAR6edYOyGuWiHIPt0nT+KFnnd1AbjZ9nzdn/gbDTezg2/rKXXLNOQ9xCT9e6Czhqy4K2A24dbFUNlQ0= X-Received: by 2002:a17:90b:4a90:b0:237:2eed:6610 with SMTP id lp16-20020a17090b4a9000b002372eed6610mr2067423pjb.4.1677160582511; Thu, 23 Feb 2023 05:56:22 -0800 (PST) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 References: <202302202112.31KLCfQB080359@gitrepo.freebsd.org> In-Reply-To: From: Rick Macklem Date: Thu, 23 Feb 2023 05:56:06 -0800 Message-ID: Subject: Re: git: ef6fcc5e2b07 - main - nfsd: Add VNET_SYSUNINIT() macros for vnet cleanup To: Gleb Smirnoff Cc: Rick Macklem , bz@freebsd.org, jamie@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4PMvh80pc4z4JJ5 X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; TAGGED_FROM(0.00)[] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N On Wed, Feb 22, 2023 at 10:28 PM Gleb Smirnoff 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. > Yes, but the sysctls are mostly useless anyhow. I don't know how to make them work in a prison. (I know how to use SYSCTL_FLAG_VNET, but that does not work in this case.) > 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. > Not if you want to keep the vnet footprint small. This was necessary so that nfsd.ko (and friends) will load dynamically. Without the conversion to mallocs, it would complain the vnet was out of memory when nfsd.ko tried to load. (I'm sure I didn't need to do all of them, but it made sense to keep the vnet footprint as small as possible.) > 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 > I'll look at it, but if it stops malloc'ng softc, then I will be worried w.r.t. vnet footprint and dynamic loading. (Note that the structure has an array of hash list pointers in it, so it is rather large. Another reason (along with dynamic loading of the modules) for keeping the foot print small is to try and keep the jails that do not use nfsd(8) and friends lightweight. When I originally coded it, I put it under a kernel option called VNET_NFSD (still in kern_jail.c at this time), but others felt that a new kernel option wasn't desirable, (There is a lot of discussion under D37519. The downside of discussions that happen in phabricator is that not as many people see them. I started an email thread on freebsd-current@, but it quickly migrated to D37519. Just the way things currently happen.) > 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. > Nope. "struct nfsstatsv1" is a structure that is shared with the NFS client, which is not vnet'd. As such, a static "struct nfsstatsv1" needs to exist for prison0. My original solution was to create a separate structure for the server side stuff (vnet it), but then the result was a messy copying exercise for the system call, which returns the entire structure (with client and server info). Once the client side is vnet'd (I've already had a couple of emails asking me to do it, so I plan on starting to work on it) then, yes, the structure can become a malloc'd vnet'd one and the IS_DEFAULT_VNET can go away. I am not sure why you think IS_DEFAULT_VNET() is such a big deal in the initialization code? It work. I can see an argument for using both SYSINIT() and VNET_SYSINIT() for cases where you have both non-vnet;d and vnet'd data to initialize. However, I don't see any problem with using IS_DEFAULT_VNET() when needed. > 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. > Ok, I was not aware of these. Since the things that are accessed frequently by threads are malloc'd (various lists off of hash tables that are also malloc'd so that they can be sized by tunables), I don't think they will be useful. The data is global to all nfsd threads and not thread specific. W.r.t. stats, it is one structure used for all threads (client and server) and is only updated once per RPC, so I don't see how these are useful there either. But I will take a look. > Please fill free to commandeer those reviews from me and finalize them if you > agree with general idea. > > -- > Gleb Smirnoff