Message-ID: <9de8d876-5729-454b-bf8c-8b0ec8f8ffc1@nvidia.com>
Date: Fri, 25 Oct 2024 13:31:49 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
CC: Pedro Falcato <pedro.falcato@...il.com>, Christian Brauner
	<christian@...uner.io>, Shuah Khan <shuah@...nel.org>, "Liam R . Howlett"
	<Liam.Howlett@...cle.com>, Suren Baghdasaryan <surenb@...gle.com>, "Vlastimil
 Babka" <vbabka@...e.cz>, <linux-kselftest@...r.kernel.org>,
	<linux-mm@...ck.org>, <linux-fsdevel@...r.kernel.org>,
	<linux-api@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Oliver Sang
	<oliver.sang@...el.com>
Subject: Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own
 thread/process

On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
> On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
>> On 10/25/24 11:38 AM, Pedro Falcato wrote:
>>> On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@...dia.com> wrote:
...
>>> That seems to only apply to the kernel internally, uapi headers are
>>
>> Yes.
>>
>>> included from userspace too (-std=c89 -pedantic doesn't know what a
>>> gnu extension is). And uapi headers _generally_ keep to defining
>>> constants and structs, nothing more.
>>
>> OK
> 
> Because a lot of people using -ANSI- C89 are importing a very new linux
> feature header.

I'll admit to being easily cowed by "you're breaking userspace" arguments.
Even when they start to get rather absurd. Because I can't easily tell where
the line is.

Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it
to be! :)

> 
> And let's ignore the hundreds of existing uses... OK.
> 
> The rules, unstated anywhere, are that we must support 1972-era C in an
> optional header for a feature available only in new kernels because
> somebody somewhere is using a VAX-11 and gosh darn it they can't change
> their toolchain!
> 
> And you had better make sure you don't wear out those tape drums...
> 
>>
>>> I don't know what the guidelines for uapi headers are nowadays, but we
>>> generally want to not break userspace.
>>>
>>>>
>>>> I think it's quite clear at this point, that we should not hold up new
>>>> work, based on concerns about handling the inline keyword, nor about
>>>> C89.
>>>
>>> Right, but the correct solution is probably to move
>>> pidfd_is_self_sentinel to some other place, since it's not even
>>> supposed to be used by userspace (it's semantically useless to
>>> userspace, and it's only two users are in the kernel, kernel/pid.c and
>>> exit.c).
>>>
>>
>> Yes, if userspace absolutely doesn't need nor want this, then putting
>> it in a non-uapi header does sound like the right move.
> 
> The bike shed should be blue! Wait no no, it should be red... Hang on
> yellow yes! Yellow's great!

Putting a header in the right location, so as to avoid breakage here or
there, is not bikeshedding. Sorry.

> 
> No wait - did we _test_ yellow in the way I wanted...
> 
> I mean for me this isn't a big deal - we declare the defines here, it makes
> sense to have a very very simple inline function.
> 
> It's not like userspace is overly hurt by this...
> 
> Also I did explain there's no obvious header to put this in in the kernel
> and I'm not introducing one sorry.
> 
> ANyway if you guys feel strong enough about this, I'll respin again and
> just open-code this trivial check where it's used.

No strong feelings, just hoping to help make a choice that gets you
closer to getting your patches committed.



thanks,
-- 
John Hubbard

Powered by blists - more mailing lists