Message-ID: <16e58180-16c1-45b9-b93b-e722bf686b43@lucifer.local>
Date: Fri, 25 Oct 2024 23:17:59 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: John Hubbard <jhubbard@...dia.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 Fri, Oct 25, 2024 at 02:51:29PM -0700, John Hubbard wrote:
> On 10/25/24 2:09 PM, Lorenzo Stoakes wrote:
> > On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
> > > 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:
> > > ...
> > > 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! :)
> >
> > Well, apparently not...
>
> Why not? Your arguments are clear and reasonable. Why shouldn't they prevail?
>
> Please don't think that I have some sort of firm position here. I'm simply
> looking for the right answer. And if that's different than something I
> proposed earlier, no problem. The best answer should win.


>
> ...
> > > > 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.
> >
> > There are 312 uses of "static inline" already in UAPI headers, not all
> > quite as obscure as claimed.
> >
>
> OK, good. Let's lead with that. It seems very clear, then, that a new one
> won't cause a problem.

Right, sorry I thought I had made this point earlier, perhaps in a sub-thread of
a thread of thread. It felt as if you guys were acting as if that were
immaterial, which is why I highlighted it again.

>
> > Specifically requiring me and only me to support ansi C89 for a theorised
> > scenario is in my opinion bikeshedding, but I don't want to get into an
> > argument about something so petty :)
>
> An argument about the definition of bikeshedding sounds delightfully
> recursive, but yes, let's not. :)

:)

>
> ...
> > > > 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.
> >
> > I mean, you are saying I am breaking things and implying the series is
> > blocked on this, that sounds like a strong opinion, but again I'm not going
> > to argue.
>
> Actually, Pedro's request kicked this off, and I was hoping to dismiss
> it--again, in order to help move things along. My opinion is that we
> should shun ancient toolchains and ancient systems whenever possible.
>
> Somehow that got turned into "I'm trying to block the patchset". Really,
> whatever works, follows The Rules (whatever we eventually understand
> them to be), and doesn't cause someone *else* to come out of the
> woodwork and claim a problem, is fine with me.

Text is a poor medium, sorry!

I don't mean to say you're doing that purposefully on any level, I mean to
say that, by arguing here over something that feels kind of unimportant, we
are inadvertently doing that.

>
> >
> > As with the requirement that I, only for my part of the change, must fix up
> > test header import, while I disagree I should be doing the fix, I did it
> > anyway as I am accommodating and reasonable.
>
> I agree that pre-existing problems in selftests should not be your
> problem.
>
> By the way, I'm occasionally involved in helping fix up various
> selftest-related problems, especially when they impact mm. Send me a
> note if you have anything in mind that ought to be fixed up, I might be
> able to help head off future grief in that area.

Sure, and I'm passionate about tests (I've written _thousands_ of lines of
tests recently!) I mention this as a related example of something that
feels out of scope.

Equally as the pidfd.h test header already had other instances of exactly
what I did and thus really should have been solved as a separate series
(one that I'd have been happy to do myself), I feel this issue, if truly a
problem should be considered separately.

>
> >
> > So fine - I'll respin and just open-code this as it's trivial and there's
> > no (other) sensible place to put it anyway.
> >
> > A P.S. though - a very NOT theoretical issue with userspace is the import
> > of linux/fcntl.h in pidfd.h which seems to me to have been imported solely
> > for the kernel's sake.
> >
> > A gentle suggestion (it seems I can't win - gentle suggestions are ignored,
> > tongue-in-cheek parody is taken to be mean... but anyway) is to do
>
> Actually, these come across as sarcasm, especially in the context of
> these emails that show you are becoming quite distraught.

Yes, I get that, rather a Brit element to this, to be clear - I am not
distraught, merely mildly perturbed. Again text is a bloody awful medium!

This genuinely was meant to be tongue in cheek, BUT I realise it's a me
issue on this kind of thing - obviously written down like that it comes off
as possibly dripping with a kind of venom that was ABSOLUTELY not intended.

Whereas my intent was a sort of wry smile, 'come on guys this doesn't
matter' thing.

But since this is the second time now that I've said something intended
that way and having been received as quite something different - this is a
me thing - and I will refrain from dalliances into the rhetorical like this
in future!

Apologies if either you or Pedro took offence and I'm fine! Other than this
damn cold that wont' go away...


>
> I've met you several times at the conferences. We get along well. And
> your work is top notch. So please consider that I'm very much supportive
> of you and your work here.

And I consider you one of the loveliest people in the kernel and very very
sharp, and have enjoyed meeting you and your erstwhile colleague Jason :)

To be clear - I also have high regard for Pedro who I consider very smart,
and I don't say that lightly.

I mean this _whole series_ is his idea, for instance. I don't just write
series based on an idea on review for just anyone ;)

So this is not at all intended to be a critique of either of you, as I have
the utmost regard for you both...!

>
> I'm still trying to understand why you are recently sending these very
> strong emails (Vlastimil also took some heat), but I see that you also
> mentioned some long hours.

Well some of higher 'strength' have more basis than others that may be my
failing to communicate things quite as intended. We'd have to speak on a
case-by-case basis :)

But in Vlastimil's case, that was absolutely not intended. Again text is a
bad medium!

>
> If my feedback is making things worse here, I'll try to adjust.
> Selftests in general are a frustrating area.

No it's fine, I think just a comms thing here.

Please do carry on reviewing it is all much appreciated... I promise!

>
>
> thanks,
> --
> John Hubbard
>
>
> > something like:
> >
> > #ifdef __KERNEL__
> > #include <linux/fcntl.h>
> > #else
> > #include <fcntl.h>
> > #endif
> >
> > At the top of the pidfd.h header. This must surely sting a _lot_ of people
> > in userland otherwise.
> >
> > But this is out of scope for this change.
>

Anyway on this issue, as I said, and meant - I will respin with this taken
out to alleviate concerns.

The _far_ more pressing issue I think is the one Pedro raised about the
actual PIDFD_SELF* values. I may simply choose some arbitrary ones in the
range specified by Pedro on respin.


Thanks! And I guess I owe you both beers ;)

Powered by blists - more mailing lists