From nobody Mon Feb 14 14:31:16 2022 X-Original-To: dev-commits-src-all@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 363A7194D280; Mon, 14 Feb 2022 14:31:19 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com [IPv6:2a00:1450:4864:20::22c]) (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 4Jy6930b0lz57M8; Mon, 14 Feb 2022 14:31:19 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lj1-x22c.google.com with SMTP id n24so19814114ljj.10; Mon, 14 Feb 2022 06:31:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=mYjWbZwpM2cSlHW6Xlou+pxMNNoIULqmMkewaFckJwc=; b=kDO226uessJdAqS+VqB+u+Q97xcyMS2fQ1kdP2yxV282C+d/lBBCBlwuT2CUbOH7IE jHNjgPg+sWsHWNANvE4EeADFGVYO9S6MaKse//YmSWDWpt1Jce8YYlvxjg5tzR6tYGqa FjFcUgmBbTZcIRpBn6gB2zR6M7bwVR91gHfuCxeoqs6W3N5p8YgodypoRpTx33bahmNP fuVkNCmJID7SEDfXrm172VY725IM/M89dtogLQ+kO9oFm8eZeRfUdJvPG5bnul62jsy9 JSHulP58vAeonaa2/HHisXUQSSjXXj3WH8RYRbENObGsD0FkFFqsqMc8JYhxKANltpw2 /Bww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=mYjWbZwpM2cSlHW6Xlou+pxMNNoIULqmMkewaFckJwc=; b=D9t3ykjiyiwA6dL0WIMuQKHStvxMS3bi3Q9LH9RVgz6bGi6O1OJpniDpOKIIJ0Ue8k SwMb6i+/Jwnt7+p1gG9IRszAkmMkRNBm/eMhJdrHmoq29lyd81RBz8ZOQ5gACPTeAblq K3XPbeGIETGMZjP/jt+nnjvgkpvv6U707nWK29kx59wWrcVcSehPb94QpPGNl3A+Qsn/ l6OoDiUIsfzwYQ3bKz4HcEwGotg1tzUsQj8JqSUP7fksMEgjJDWRfYeKvhMf0xOn0mDT LcS3M4dWJ5S30aSTPPPVEgax9Se813021TIlcdrPwJjcj1DXq4nZ6A+QVH/s7BNGgFtd GrlQ== X-Gm-Message-State: AOAM533IJlb+5ymU3amxmU+ZvkPsn9JlCDGoLbfcEMSxHyU8b7t0L9jl 0vL0+S1otHmrgoRzQLe0S8dOj92Msa4v5IuS6inbMExJ X-Google-Smtp-Source: ABdhPJylaCKrutksY7aMC72XvB5UjdS/QiaqRAoYGYFLWCpogCM4Ea+VkQXiyCxwf+9LKhZMmEBeuRGXcd0iivvnnu8= X-Received: by 2002:a2e:9056:: with SMTP id n22mr6728577ljg.503.1644849077626; Mon, 14 Feb 2022 06:31:17 -0800 (PST) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Received: by 2002:a05:6520:1c7:b0:19c:42d9:8e5d with HTTP; Mon, 14 Feb 2022 06:31:16 -0800 (PST) In-Reply-To: References: <202202111357.21BDvxhr093033@gitrepo.freebsd.org> From: Mateusz Guzik Date: Mon, 14 Feb 2022 15:31:16 +0100 Message-ID: Subject: Re: git: 32114b639fa1 - main - Add PROC_COW_CHANGECOUNT and thread_cow_synced To: Konstantin Belousov Cc: 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: 4Jy6930b0lz57M8 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On 2/13/22, Konstantin Belousov wrote: > On Sat, Feb 12, 2022 at 10:22:28PM +0100, Mateusz Guzik wrote: >> On 2/12/22, Konstantin Belousov wrote: >> > On Sat, Feb 12, 2022 at 07:50:21PM +0100, Mateusz Guzik wrote: >> >> On 2/11/22, Konstantin Belousov wrote: >> >> > On Fri, Feb 11, 2022 at 01:57:59PM +0000, Mateusz Guzik wrote: >> >> >> The branch main has been updated by mjg: >> >> >> >> >> >> URL: >> >> >> https://cgit.FreeBSD.org/src/commit/?id=32114b639fa1ad777312eebe14a9f677bd7be2ea >> >> >> >> >> >> commit 32114b639fa1ad777312eebe14a9f677bd7be2ea >> >> >> Author: Mateusz Guzik >> >> >> AuthorDate: 2022-02-01 13:13:13 +0000 >> >> >> Commit: Mateusz Guzik >> >> >> CommitDate: 2022-02-11 11:44:07 +0000 >> >> >> >> >> >> Add PROC_COW_CHANGECOUNT and thread_cow_synced >> >> >> >> >> >> Combined they can be used to avoid a proc lock/unlock cycle in >> >> >> the >> >> >> syscall handler for curthread, see upcoming examples. >> >> >> --- >> >> >> sys/kern/kern_thread.c | 13 +++++++++++++ >> >> >> sys/sys/proc.h | 9 +++++++++ >> >> >> 2 files changed, 22 insertions(+) >> >> >> >> >> >> diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c >> >> >> index dcb52b137b58..bb724a17803e 100644 >> >> >> --- a/sys/kern/kern_thread.c >> >> >> +++ b/sys/kern/kern_thread.c >> >> >> @@ -868,6 +868,19 @@ thread_cow_update(struct thread *td) >> >> >> lim_free(oldlimit); >> >> >> } >> >> >> >> >> >> +void >> >> >> +thread_cow_synced(struct thread *td) >> >> >> +{ >> >> >> + struct proc *p; >> >> >> + >> >> >> + p = td->td_proc; >> >> >> + PROC_LOCK_ASSERT(p, MA_OWNED); >> >> >> + MPASS(td->td_cowgen != p->p_cowgen); >> >> >> + MPASS(td->td_ucred == p->p_ucred); >> >> >> + MPASS(td->td_limit == p->p_limit); >> >> >> + td->td_cowgen = p->p_cowgen; >> >> > This should be store-release, I think. >> >> > And corresponding loads in trap() needs to get acquire semantic. >> >> > >> >> > This is probably a pre-existing bug. >> >> >> >> I don't think adding fences would improve anything here. First note >> >> fences or not, the thread can still race against cowgen changing and >> >> miss it this time around. At the same time all updates to cowgen are >> >> done with process lock, which will also be taken to sync. Consequently >> >> the thread at hand in the worst case will miss cowgen being updated >> >> and will act on it next time. If it decides to act on cowgen, it takes >> >> the lock which guarantees everything is stable. >> > If thread missed generation update, it is it. >> > >> > Fence would handle the other case, when the thread observed cowgen >> > udate, >> > but continue to use old cow values. >> > >> > The process lock does not help there at all. >> > >> >> What do you mean by 'cow values'? You mean the pointers like p_ucred? >> >> td_ucred and td_limit are only modified by curthread, thus they don't >> require synchronization. p_ucred and p_limit are only accessed with >> the lock held, which provides the necessary synchronization. The only >> thing inspected locklessly is the cowgen counter, which is safe to >> race against. > You are talking about removing the process lock, right in the commit > message. The only lock case related to cow values is around reading of > the p_ pointers, to stash them into td_. No, see 93288e2445fea95cb15b266759c2d9e382ea4e47 as an example consumer. All work is still done with the process lock held. The difference is that if the change at hand is the only one which needs syncing for curthread, curthread can sync it immediately and end up with up-to-date cowgen, consequently if no other changes pop up next time it checks cowgen, it will avoid calling thread_cow_update and the proc lock/unlock trip. >> >> >> >> >> The code definitely should use atomic_store/load_int though, but there >> >> are numerous bugs of this sort all over, so I don't think this is >> >> pressing. >> >> >> >> > >> >> >> +} >> >> >> + >> >> >> /* >> >> >> * Discard the current thread and exit from its context. >> >> >> * Always called with scheduler locked. >> >> >> diff --git a/sys/sys/proc.h b/sys/sys/proc.h >> >> >> index ff97bfbd54a9..0e33192303f4 100644 >> >> >> --- a/sys/sys/proc.h >> >> >> +++ b/sys/sys/proc.h >> >> >> @@ -1009,6 +1009,14 @@ extern pid_t pid_max; >> >> >> (p)->p_cowgen++; \ >> >> >> } while (0) >> >> >> >> >> >> +#define PROC_COW_CHANGECOUNT(td, p) ({ \ >> >> >> + struct thread *_td = (td); \ >> >> >> + struct proc *_p = (p); \ >> >> >> + MPASS(_td == curthread); \ >> >> >> + PROC_LOCK_ASSERT(_p, MA_OWNED); \ >> >> >> + _p->p_cowgen - _td->td_cowgen; \ >> >> >> +}) >> >> >> + >> >> >> /* Check whether a thread is safe to be swapped out. */ >> >> >> #define thread_safetoswapout(td) ((td)->td_flags & TDF_CANSWAP) >> >> >> >> >> >> @@ -1200,6 +1208,7 @@ void thread_cow_get_proc(struct thread >> >> >> *newtd, >> >> >> struct proc *p); >> >> >> void thread_cow_get(struct thread *newtd, struct thread *td); >> >> >> void thread_cow_free(struct thread *td); >> >> >> void thread_cow_update(struct thread *td); >> >> >> +void thread_cow_synced(struct thread *td); >> >> >> int thread_create(struct thread *td, struct rtprio *rtp, >> >> >> int (*initialize_thread)(struct thread *, void *), void >> >> >> *thunk); >> >> >> void thread_exit(void) __dead2; >> >> > >> >> >> >> >> >> -- >> >> Mateusz Guzik >> > >> >> >> -- >> Mateusz Guzik > -- Mateusz Guzik