From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> To: Andrea Parri <parri.andrea@gmail.com> Cc: paulmck@kernel.org, palmer@dabbelt.com, paul.walmsley@sifive.com, aou@eecs.berkeley.edu, mmaas@google.com, hboehm@google.com, striker@us.ibm.com, charlie@rivosinc.com, rehn@rivosinc.com, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] membarrier: riscv: Provide core serializing command Date: Wed, 29 Nov 2023 15:00:53 -0500 [thread overview] Message-ID: <53aac2ac-46ae-46b8-9fdf-34527b79a63b@efficios.com> (raw) In-Reply-To: <ZWeDF0eHyOc/b9UJ@andrea> On 2023-11-29 13:29, Andrea Parri wrote: >>> diff --git a/arch/riscv/mm/context.c b/arch/riscv/mm/context.c >>> index 217fd4de61342..f63222513076d 100644 >>> --- a/arch/riscv/mm/context.c >>> +++ b/arch/riscv/mm/context.c >>> @@ -323,6 +323,23 @@ void switch_mm(struct mm_struct *prev, struct mm_struct *next, >>> if (unlikely(prev == next)) >>> return; >>> +#if defined(CONFIG_MEMBARRIER) && defined(CONFIG_SMP) >>> + /* >>> + * The membarrier system call requires a full memory barrier >>> + * after storing to rq->curr, before going back to user-space. >>> + * >>> + * Only need the full barrier when switching between processes: >>> + * barrier when switching from kernel to userspace is not >>> + * required here, given that it is implied by mmdrop(); barrier >>> + * when switching from userspace to kernel is not needed after >>> + * store to rq->curr. >>> + */ >>> + if (unlikely(atomic_read(&next->membarrier_state) & >>> + (MEMBARRIER_STATE_PRIVATE_EXPEDITED | >>> + MEMBARRIER_STATE_GLOBAL_EXPEDITED)) && prev) >>> + smp_mb(); >>> +#endif >> >> The approach looks good. Please implement it within a separate >> membarrier_arch_switch_mm() as done on powerpc. > > Will do. Thanks. > > As regards the Fixes: tag, I guess it boils down to what we want or we > need to take for commit "riscv: Support membarrier private cmd". :-) I'm not seeing this commit in the Linux master branch, am I missing something ? > FWIW, a quick git-log search confirmed that MEMBARRIER has been around > for quite some time in the RISC-V world (though I'm not familiar with > any of its mainstream uses): commit 1464d00b27b2 says (at least) since > 93917ad50972 ("RISC-V: Add support for restartable sequence"). I am > currently inclined to pick the latter commit (and check it w/ Palmer), > but other suggestions are welcome. Supporting membarrier private expedited is not optional since Linux 4.14: see kernel/sched/core.c: membarrier_switch_mm(rq, prev->active_mm, next->mm); /* * sys_membarrier() requires an smp_mb() between setting * rq->curr / membarrier_switch_mm() and returning to userspace. * * The below provides this either through switch_mm(), or in * case 'prev->active_mm == next->mm' through * finish_task_switch()'s mmdrop(). */ switch_mm_irqs_off(prev->active_mm, next->mm, next); Failure to provide the required barrier is a bug in the architecture's switch_mm implementation when CONFIG_MEMBARRIER=y. We should probably introduce a new Documentation/features/sched/membarrier/arch-support.txt to clarify this requirement. Userspace code such as liburcu [1] heavily relies on membarrier private expedited (when available) to speed up RCU read-side critical sections. Various DNS servers, including BIND 9, use liburcu. Thanks, Mathieu [1] https:/liburcu.org -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
WARNING: multiple messages have this Message-ID (diff)
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> To: Andrea Parri <parri.andrea@gmail.com> Cc: paulmck@kernel.org, palmer@dabbelt.com, paul.walmsley@sifive.com, aou@eecs.berkeley.edu, mmaas@google.com, hboehm@google.com, striker@us.ibm.com, charlie@rivosinc.com, rehn@rivosinc.com, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] membarrier: riscv: Provide core serializing command Date: Wed, 29 Nov 2023 15:00:53 -0500 [thread overview] Message-ID: <53aac2ac-46ae-46b8-9fdf-34527b79a63b@efficios.com> (raw) In-Reply-To: <ZWeDF0eHyOc/b9UJ@andrea> On 2023-11-29 13:29, Andrea Parri wrote: >>> diff --git a/arch/riscv/mm/context.c b/arch/riscv/mm/context.c >>> index 217fd4de61342..f63222513076d 100644 >>> --- a/arch/riscv/mm/context.c >>> +++ b/arch/riscv/mm/context.c >>> @@ -323,6 +323,23 @@ void switch_mm(struct mm_struct *prev, struct mm_struct *next, >>> if (unlikely(prev == next)) >>> return; >>> +#if defined(CONFIG_MEMBARRIER) && defined(CONFIG_SMP) >>> + /* >>> + * The membarrier system call requires a full memory barrier >>> + * after storing to rq->curr, before going back to user-space. >>> + * >>> + * Only need the full barrier when switching between processes: >>> + * barrier when switching from kernel to userspace is not >>> + * required here, given that it is implied by mmdrop(); barrier >>> + * when switching from userspace to kernel is not needed after >>> + * store to rq->curr. >>> + */ >>> + if (unlikely(atomic_read(&next->membarrier_state) & >>> + (MEMBARRIER_STATE_PRIVATE_EXPEDITED | >>> + MEMBARRIER_STATE_GLOBAL_EXPEDITED)) && prev) >>> + smp_mb(); >>> +#endif >> >> The approach looks good. Please implement it within a separate >> membarrier_arch_switch_mm() as done on powerpc. > > Will do. Thanks. > > As regards the Fixes: tag, I guess it boils down to what we want or we > need to take for commit "riscv: Support membarrier private cmd". :-) I'm not seeing this commit in the Linux master branch, am I missing something ? > FWIW, a quick git-log search confirmed that MEMBARRIER has been around > for quite some time in the RISC-V world (though I'm not familiar with > any of its mainstream uses): commit 1464d00b27b2 says (at least) since > 93917ad50972 ("RISC-V: Add support for restartable sequence"). I am > currently inclined to pick the latter commit (and check it w/ Palmer), > but other suggestions are welcome. Supporting membarrier private expedited is not optional since Linux 4.14: see kernel/sched/core.c: membarrier_switch_mm(rq, prev->active_mm, next->mm); /* * sys_membarrier() requires an smp_mb() between setting * rq->curr / membarrier_switch_mm() and returning to userspace. * * The below provides this either through switch_mm(), or in * case 'prev->active_mm == next->mm' through * finish_task_switch()'s mmdrop(). */ switch_mm_irqs_off(prev->active_mm, next->mm, next); Failure to provide the required barrier is a bug in the architecture's switch_mm implementation when CONFIG_MEMBARRIER=y. We should probably introduce a new Documentation/features/sched/membarrier/arch-support.txt to clarify this requirement. Userspace code such as liburcu [1] heavily relies on membarrier private expedited (when available) to speed up RCU read-side critical sections. Various DNS servers, including BIND 9, use liburcu. Thanks, Mathieu [1] https:/liburcu.org -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-11-29 20:01 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-11-27 10:32 [PATCH 0/2] membarrier: riscv: Provide core serializing command Andrea Parri 2023-11-27 10:32 ` Andrea Parri 2023-11-27 10:32 ` [PATCH 1/2] locking: Introduce prepare_sync_core_cmd() Andrea Parri 2023-11-27 10:32 ` Andrea Parri 2023-11-27 12:53 ` Mathieu Desnoyers 2023-11-27 12:53 ` Mathieu Desnoyers 2023-11-27 10:32 ` [PATCH 2/2] membarrier: riscv: Provide core serializing command Andrea Parri 2023-11-27 10:32 ` Andrea Parri 2023-11-27 13:28 ` Mathieu Desnoyers 2023-11-27 13:28 ` Mathieu Desnoyers 2023-11-28 15:13 ` Andrea Parri 2023-11-28 15:13 ` Andrea Parri 2023-11-28 18:39 ` Mathieu Desnoyers 2023-11-28 18:39 ` Mathieu Desnoyers 2023-11-29 18:29 ` Andrea Parri 2023-11-29 18:29 ` Andrea Parri 2023-11-29 20:00 ` Mathieu Desnoyers [this message] 2023-11-29 20:00 ` Mathieu Desnoyers 2023-11-29 21:25 ` Andrea Parri 2023-11-29 21:25 ` Andrea Parri 2023-11-29 21:32 ` Mathieu Desnoyers 2023-11-29 21:32 ` Mathieu Desnoyers 2023-11-29 22:43 ` Andrea Parri 2023-11-29 22:43 ` Andrea Parri 2023-12-06 13:05 ` Palmer Dabbelt 2023-12-06 13:05 ` Palmer Dabbelt 2023-12-06 14:11 ` Andrea Parri 2023-12-06 14:11 ` Andrea Parri 2023-12-06 14:15 ` Palmer Dabbelt 2023-12-06 14:15 ` Palmer Dabbelt 2023-12-06 17:42 ` Andrea Parri 2023-12-06 17:42 ` Andrea Parri 2023-12-06 17:56 ` Palmer Dabbelt 2023-12-06 17:56 ` Palmer Dabbelt
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=53aac2ac-46ae-46b8-9fdf-34527b79a63b@efficios.com \ --to=mathieu.desnoyers@efficios.com \ --cc=aou@eecs.berkeley.edu \ --cc=charlie@rivosinc.com \ --cc=hboehm@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=mmaas@google.com \ --cc=palmer@dabbelt.com \ --cc=parri.andrea@gmail.com \ --cc=paul.walmsley@sifive.com \ --cc=paulmck@kernel.org \ --cc=rehn@rivosinc.com \ --cc=striker@us.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.