Re: git: 4a69fc16a583 - main - Add membarrier(2)

On Wed, Aug 23, 2023 at 02:18:12AM +0100, Jessica Clarke wrote:
> On 23 Aug 2023, at 01:07, Konstantin Belousov <kib@FreeBSD.org> wrote:
> > 
> > The branch main has been updated by kib:
> > 
> > URL: https://cgit.FreeBSD.org/src/commit/?id=4a69fc16a583face922319c476f3e739d9ce9140
> > 
> > commit 4a69fc16a583face922319c476f3e739d9ce9140
> > Author:     Konstantin Belousov <kib@FreeBSD.org>
> > AuthorDate: 2021-10-07 21:10:07 +0000
> > Commit:     Konstantin Belousov <kib@FreeBSD.org>
> > CommitDate: 2023-08-23 00:02:21 +0000
> > 
> >    Add membarrier(2)
> > 
> >    This is an attempt at clean-room implementation of the Linux'
> >    membarrier(2) syscall.  For documentation, you would need to read
> >    both membarrier(2) Linux man page, the comments in Linux
> >    kernel/sched/membarrier.c implementation and possibly look at
> >    actual uses.
> > 
> >    Sponsored by:   The FreeBSD Foundation
> >    MFC after:      1 week
> >    Differential revision:  https://reviews.freebsd.org/D32360
> > ---
> > ...
> > diff --git a/sys/sys/membarrier.h b/sys/sys/membarrier.h
> > new file mode 100644
> > index 000000000000..958b769da23e
> > --- /dev/null
> > +++ b/sys/sys/membarrier.h
> > @@ -0,0 +1,70 @@
> > +/*-
> > + * Copyright (c) 2021 The FreeBSD Foundation
> > + *
> > + * This software were developed by Konstantin Belousov <kib@FreeBSD.org>
> > + * under sponsorship from the FreeBSD Foundation.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > + * SUCH DAMAGE.
> > + */
> > +
> > +#ifndef __SYS_MEMBARRIER_H__
> > +#define __SYS_MEMBARRIER_H__
> > +
> > +#include <sys/cdefs.h>
> > +
> > +/*
> > + * The enum membarrier_cmd values are bits.  The MEMBARRIER_CMD_QUERY
> > + * command returns a bitset indicating which commands are supported.
> > + * Also the value of MEMBARRIER_CMD_QUERY is zero, so it is
> > + * effectively not returned by the query.
> > + */
> > +enum membarrier_cmd {
> > + MEMBARRIER_CMD_QUERY = 0x00000000,
> > + MEMBARRIER_CMD_GLOBAL = 0x00000001,
> > + MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
> > + MEMBARRIER_CMD_GLOBAL_EXPEDITED = 0x00000002,
> > + MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED = 0x00000004,
> > + MEMBARRIER_CMD_PRIVATE_EXPEDITED = 0x00000008,
> > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED = 0x00000010,
> > + MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE = 0x00000020,
> > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE = 0x00000040,
> > +
> > + /*
> > + * RSEQ constants are defined for source compatibility but are
> > + * not yes supported, MEMBARRIER_CMD_QUERY does not return
> > + * them in the mask.
> > + */
> > + MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ = 0x00000080,
> > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ = 0x00000100,
> 
> Or we could not define them. membarrier(2) came into Linux in 4.3, but
> these two are only since 5.10. Defining something we may or may not
> ever implement that code should have compatibility for anyway seems
> like a bad idea. Or should we provide all of the Linux UAPI to FreeBSD
> processes with ENOSYS stubs? Clearly not. So why is this special? IMO
> these should not be added unless we have an rseq implementation, which
> has itself received several objections on the review.
The added _RSEQ bits behave according to the membarrier(2) specification,
they are defined but not returned in the mask of supported operations.

I found that have all of them defined makes compiling some program easier,
AFAIR.

> 
> I also don’t see why this is suddenly being landed now without warning.
> There’s been no activity by you on the review since just over a year
> ago. And landing new syscalls days before the 14 branch point doesn’t
> seem like great practice to me...
Would I know that reviewers did not act for a year, I committed the patch
a year ago.

The addition does not change any existing code path in the kernel.