From nobody Sat Feb 12 18:50:21 2022 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 6F6EF19B140A; Sat, 12 Feb 2022 18:50:31 +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 4Jx0125S48z3GrY; Sat, 12 Feb 2022 18:50:30 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lj1-x22c.google.com with SMTP id j14so16814133lja.3; Sat, 12 Feb 2022 10:50:30 -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=MFNm7Zezy/x0ymVIJl77Wb9XTs0eAiTqWZtPTqSs9qE=; b=F8OIVzxRk/qkOGl2Or2pEYKPLXGdGWgaKHP1VP2RFx0xEO72LErdziqqUlojv400bi HgzNxbtXIfQAL658dTjuZuDD4l5Ug9eUUyI4BcK9YxPrGvh2ftYUtnwrhYirTZ7E815b fV9dZnjGq0N4TyuPLWtsD42xhv8ft2QnY3N5A47wHLsW7FdiQOw25Ur8T0J1iGTcrNce WBKRS/GoCXRhfBlF5wxmro9ge4vBB8pfGpv3RZ9KLzGUFM7zDzBTrzIWmJbU4Z1kZuBq xY0GgoNFcjgrPMJtk8VdKza9IK4TxVIKJA131SJqAW7KnFS/069XFQ8qN5KZLrllxhEc OcHQ== 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=MFNm7Zezy/x0ymVIJl77Wb9XTs0eAiTqWZtPTqSs9qE=; b=QR5tOaXK+Urv9OK9nrDTWo3zDTNEr5ZSr+Q0j/5BAvvCPhng+tMbOdJbNrweDZDJxn tHAuRTg47+Lq50tmvg3pv7ZurBpKNEsIykOkkYMdu8+YH+/oOO0P8hdtnBdNS5UCrrqd v0AqORtdUpdAI5dXRXnpwe3sMUJO7Q17IYY97V5/rNibcMNolZJQHuzQWotXEFbKytox wmergoJ48ZPx9/dCKmLt8/R1jyHO/XCpdpQkxJ/jm2ynFEuwelnjXlyQwXM5XoQksYdc 5D83JHl1abwR+sGc6O3okjVQpleKm4hs8kHrCEHZxA9gvBygbs7kAGpCajWpYsGzGWLL sZAQ== X-Gm-Message-State: AOAM531gvLTCvfCkf2KR6BxaSqVa3drH1MX96LkA99Tki6hltiRvwsoj RqkaAteo30vvYjIQS7yssX+/mBvWRMbWcnCj458= X-Google-Smtp-Source: ABdhPJwA/tOYXVj2wUSgT4y8Cejtnmtopn+32C5L1Z9eb0LhCTp1n1atYTmVVNXGFIFp0REwZ5ci1tSDrBa3i797ZDw= X-Received: by 2002:a05:651c:384:: with SMTP id e4mr2729635ljp.175.1644691822971; Sat, 12 Feb 2022 10:50: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 Received: by 2002:aa6:cb4b:0:b0:19a:8792:ef28 with HTTP; Sat, 12 Feb 2022 10:50:21 -0800 (PST) In-Reply-To: References: <202202111357.21BDvxhr093033@gitrepo.freebsd.org> From: Mateusz Guzik Date: Sat, 12 Feb 2022 19:50:21 +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: 4Jx0125S48z3GrY X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=F8OIVzxR; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::22c as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-4.00 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36:c]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[text/plain]; NEURAL_HAM_LONG(-1.00)[-1.000]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MID_RHS_MATCH_FROMTLD(0.00)[]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[2a00:1450:4864:20::22c:from]; NEURAL_HAM_SHORT(-1.00)[-1.000]; MLMMJ_DEST(0.00)[dev-commits-src-all,dev-commits-src-main]; FREEMAIL_TO(0.00)[gmail.com]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-ThisMailContainsUnwantedMimeParts: N 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. 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