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

On 23 Aug 2023, at 03:38, Konstantin Belousov <kostikbel@gmail.com> wrote:
> 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.

So would implementing io_uring_enter(2) as EOPNOTSUPP...

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

Which programs? I can’t imagine many even try and use this in the first
place, and of those that do a large proportion likely have fallback
code for pre-5.10.

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

You didn’t even ping the patch once since 16th August 2022, a day when you
did receive reviewer feedback.

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

No, but it commits us to a new syscall being stable just days before 14
branches and has its ABI frozen.

Jess