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

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.

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...

Jess