On Wed, Dec 7, 2022 at 7:04 PM Michał Cłapiński <mclapinski@xxxxxxxxxx> wrote:
>
> On Wed, Dec 7, 2022 at 6:07 PM Mathieu Desnoyers
> <mathieu.desnoyers@xxxxxxxxxxxx> wrote:
> >
> > On 2022-12-07 11:43, Michal Clapinski wrote:
> > > Provide a method to query previously issued registrations.
> > >
> > > Signed-off-by: Michal Clapinski <mclapinski@xxxxxxxxxx>
> > > ---
> > > include/uapi/linux/membarrier.h | 4 ++++
> > > kernel/sched/membarrier.c | 39 ++++++++++++++++++++++++++++++++-
> > > 2 files changed, 42 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
> > > index 737605897f36..5f3ad6d5be6f 100644
> > > --- a/include/uapi/linux/membarrier.h
> > > +++ b/include/uapi/linux/membarrier.h
> > > @@ -137,6 +137,9 @@
> > > * @MEMBARRIER_CMD_SHARED:
> > > * Alias to MEMBARRIER_CMD_GLOBAL. Provided for
> > > * header backward compatibility.
> > > + * @MEMBARRIER_CMD_GET_REGISTRATIONS:
> > > + * Returns a bitmask of previously issued
> > > + * registration commands.
> > > *
> > > * Command to be passed to the membarrier system call. The commands need to
> > > * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
> > > @@ -153,6 +156,7 @@ enum membarrier_cmd {
> > > MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 6),
> > > MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ = (1 << 7),
> > > MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ = (1 << 8),
> > > + MEMBARRIER_CMD_GET_REGISTRATIONS = (1 << 9),
>
> Btw. I could do this as a flag to MEMBARRIER_CMD_QUERY instead of a
> separate command. Would that be preferable?
>
>
> > >
> > > /* Alias for header backward compatibility. */
> > > MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
> > > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> > > index 0c5be7ebb1dc..2ad881d07752 100644
> > > --- a/kernel/sched/membarrier.c
> > > +++ b/kernel/sched/membarrier.c
> > > @@ -159,7 +159,8 @@
> > > | MEMBARRIER_CMD_PRIVATE_EXPEDITED \
> > > | MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED \
> > > | MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK \
> > > - | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
> > > + | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK \
> > > + | MEMBARRIER_CMD_GET_REGISTRATIONS)
> > >
> > > static void ipi_mb(void *info)
> > > {
> > > @@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
> > > return 0;
> > > }
> > >
> > > +static int membarrier_get_registrations(void)
> > > +{
> > > + struct task_struct *p = current;
> > > + struct mm_struct *mm = p->mm;
> > > + int registrations_mask = 0, membarrier_state, i;
> > > + static const int states[] = {
> > > + MEMBARRIER_STATE_GLOBAL_EXPEDITED |
> > > + MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,
> >
> > What is the purpose of checking for the _READY state flag as well here ?
>
> Answered below.
>
>
> >
> >
> > > + MEMBARRIER_STATE_PRIVATE_EXPEDITED |
> > > + MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
> > > + MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
> > > + MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
> > > + MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
> > > + MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
> > > + };
> > > + static const int registration_cmds[] = {
> > > + MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> > > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
> > > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
> > > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
> > > + };
> > > + BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
> > > +
> > > + membarrier_state = atomic_read(&mm->membarrier_state);
> > > + for (i = 0; i < ARRAY_SIZE(states); ++i) {
> > > + if (membarrier_state & states[i]) {
> >
> > The mask will match if either of the flags to match are set. Is that
> > your intent ?
>
> Kind of, it was just the easiest to write. As explained in the cover
> letter, I don't really care much about the result of this while the
> process is running. And when the process is frozen, either state and
> state_ready are set or none of them.
>
>
> >
> >
> > > + registrations_mask |= registration_cmds[i];
> > > + membarrier_state &= ~states[i];
> >
> > So I understand that those _READY flags are there purely for making sure
> > we clear the membarrier_state for validation that they have all been
> > checked with the following WARN_ON_ONCE(). Am I on the right track ?
>
> Yes, exactly. It wastes time but I'm worried about people adding new
> states and not updating this function. A suggestion on how to do this
> better (especially at compile time) would be greatly appreciated.
>
>
> >
> > > + }
> > > + }
> > > + WARN_ON_ONCE(membarrier_state != 0);
> >
> > Thanks,
> >
> > Mathieu
> >
> > > + return registrations_mask;
> > > +}
> > > +
> > > /**
> > > * sys_membarrier - issue memory barriers on a set of threads
> > > * @cmd: Takes command values defined in enum membarrier_cmd.
> > > @@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
> > > return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
> > > case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
> > > return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
> > > + case MEMBARRIER_CMD_GET_REGISTRATIONS:
> > > + return membarrier_get_registrations();
> > > default:
> > > return -EINVAL;
> > > }
> >
> > --
> > Mathieu Desnoyers
> > EfficiOS Inc.
> > https://www.efficios.com
> >
Hi Mathieu,
is there anything more you need from my side?
