From nobody Sat Feb 12 21:22:28 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 905DE19558C5; Sat, 12 Feb 2022 21:22:36 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) (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 4Jx3NX3MK7z3p4c; Sat, 12 Feb 2022 21:22:36 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lf1-x12f.google.com with SMTP id d33so616823lfv.10; Sat, 12 Feb 2022 13:22:36 -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=xrzj7mTDX1i7X4BKTfkN3AzD8k2s71XARUTKSwjK6j8=; b=GCRZx/RTWZN4rhE07TYStxRBysL4qVbcuCFG8F1AI3j7hMjvptQMiuFAIFHbnlTy79 hc0K2omC/IxMdV3f+uLBk3Lxljb/PMk466DqjDkrwsol2SFWz+mZOsLouMTPqRR8elhE X4wM5xXg8jKZY/YRd2IM8t2J14vXuNRIgBXCBTU/Irkd4w5ARiUbDyn3ZdcGLOKZw4mt 5VL1HFQqqP4ILjSLL2hHE4U1G9V7S0kBn5wl9/SeWQPCtxn0JmdPzEJ7e7v468IYLCzS UNl/JKVqLG8iaiuirJsJjXQbAX4l0IzocV7BhYs+lMolTAhVkyTMLS1qBKgufCMGkfg/ Jkdw== 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=xrzj7mTDX1i7X4BKTfkN3AzD8k2s71XARUTKSwjK6j8=; b=Ww9Uwxxj0YqamWXa/flmy80UFXtnqo+5/B72q9vW+XYD2s6OrKetNTB7B/1ZPvpdo0 oijtQGPLRLZPy522bijzT7awUcBPOVtlXd8dZ56jUA4RToOw2fp2E5ZL1EzM6gU6SFY+ +2S1lpt6Vn1qsC+fcbMyHqspKiDaEu5C608igGdopo+f6PJ7kbIhKVAw5TxBL+bjELUT qyL5LiprmRH3TyDJyPWtq8Iat0EUa8dB+y9uXxdF64izZLEOdYNkWVfQrEBrClKHfliM 9+5CA+cjalQU7DssHYtg5p2DoTB0a004+DEx+z4lp3j/mxOEOWwxmYk3vFXyjeHyKXVD bYww== X-Gm-Message-State: AOAM531t2egVUS3qpxhUfe58Tk8iIBeLdQC5QHvaQiy7PGyTzwk5iJ/q skHa6lSyv7sbbBf8GeYfNNCT3MRCosJLDlKrHnuFGZ2D X-Google-Smtp-Source: ABdhPJzZrPzMODeuUWyQbbRtD+Lju8YW4gtbznWYoorkfccipqwnCiPafEzlLmYRLQUr8aIvYggn8qfabpVzm0dA5gg= X-Received: by 2002:a05:6512:3d23:: with SMTP id d35mr1472377lfv.142.1644700948813; Sat, 12 Feb 2022 13:22:28 -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:aa6:cb4b:0:b0:19a:8792:ef28 with HTTP; Sat, 12 Feb 2022 13:22:28 -0800 (PST) In-Reply-To: References: <202202111357.21BDvxhr093033@gitrepo.freebsd.org> From: Mateusz Guzik Date: Sat, 12 Feb 2022 22:22:28 +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: 4Jx3NX3MK7z3p4c 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/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. >> >> 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