Message-ID: <31a007c0-884f-495d-ba27-08e3e0dd767d@lucifer.local>
Date: Thu, 20 Feb 2025 13:18:07 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Kalesh Singh <kaleshsingh@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Matthew Wilcox <willy@...radead.org>, Vlastimil Babka <vbabka@...e.cz>,
        "Paul E . McKenney" <paulmck@...nel.org>, Jann Horn <jannh@...gle.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        linux-api@...r.kernel.org, John Hubbard <jhubbard@...dia.com>,
        Juan Yescas <jyescas@...gle.com>
Subject: Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem
 mappings

On Thu, Feb 20, 2025 at 01:44:20PM +0100, David Hildenbrand wrote:
> On 20.02.25 11:15, Lorenzo Stoakes wrote:
> > On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > > > that. But maybe you and Kalesh have a history on that that let's you react
> > > > > on his questions IMHO more emotionally than it should have been.
> > > >
> > > > This is wholly unfair, I have been very reasonable in response to this
> > > > thread. I have offered to find solutions, I have tried to understand the
> > > > problem in spite of having gone to great lengths to try to discuss the
> > > > limitations of the proposed approach in every venue I possibly could.
> > > >
> > > > I go out of my way to deal professionally and objectively with what is
> > > > presented. Nothing here is emotional. So I'd ask that you please abstain
> > > > from making commentary like this which has no basis.
> > >
> > > I appreciate everything you write below. But this request is just
> > > impossible. I will keep raising my opinion and misunderstandings will
> > > happen.
> >
> > Well I wouldn't ask you not to express your opinion David, you know I respect
> > and like you, and by all means push back hard or call out what you think is bad
> > behaviour :)
> >
> > I just meant to say, in my view, that there was no basis, but I appreciate
> > miscommunications happen.
> > > So apologies if I came off as being difficult or rude, it actually
> wasn't
> > intended. And to re-emphasise - I have zero personal issue with anybody in this
> > thread whatsoever!
>
> It sounded to me like you were trying to defend your work (again, IMHO too
> emotionally, and I might have completely misinterpreted that) and slowly
> switching to "friendly fire" (towards me). Apologies from my side if I
> completely misunderstood/misinterpreted that.

Right this was not at all my intent, sorry if it seemed that way. I may well
have communicated terribly, so apologies on my side too.

>
> To recap: what we have upstream is great; you did a great job. Yes, the
> mechanism has its drawbacks, but that's just part of the design.

Thanks :)

>
> Some people maybe have wrong expectations, maybe there were
> misunderstandings, or maybe there are requirements that only now pop up;
> it's sometimes unavoidable, and that's ok.
>
> We can try to document it better (and I was trying to find clues why people
> might be mislead), and see if/how we could sort out these requirements. But
> we can likely not make it perfect in any possible way (I'm sure there are
> plenty of use cases where what we currently have is more than sufficient).

Sure and I"m very open to adding a documentation page for guard regions, in
fact was considering this very thing recently. I already added man pages
but be good to be able to go into more depth.

>
> > > I just want to find the best way forward, technically and am willing to
> do
> > whatever work is required to make the guard region implementation as good as it
> > possibly can be.
> >
> > >
> > > Note that the whole "Honestly David you and the naming. .." thing could have
> > > been written as "I don't think it's a naming problem."
> >
> > I feel like I _always_ get in trouble when I try to write in a 'tongue-in-cheek'
> > style, which is what this was meant to be... so I think herein lies the basis of
> > the miscommunication :)
> >
> > I apologise, the household is ill, which maybe affects my judgment in how I
> > write these, but in general text is a very poor medium. It was meant to be said
> > in a jolly tone with a wink...
> >
> > I think maybe I should learn my lesson with these things, I thought the ':p'
> > would make this clear but yeah, text, poor medium.
> >
> > Anyway apologies if this seemed disrespectful.
>
> No worries, it's hard to really make me angry, and I appreciate your
> openness and your apology (well, and you and your work, obviously).
>
> I'll note, though, if my memory serves me right, that nobody so far ever
> criticized the way I communicate upstream, or even told me to abstain from
> certain communication.

I wish I could say the same haha, so perhaps this was a problem on my side
honestly. I do have a habit of being 'tongue in cheek' and failing to
communicate that which I did say the last time I wouldn't repeat. It is not
intended, I promise.

As the abstain, was more a British turn of phrase, meaning to say - I
dispute the claim that this is an emotional thing and please don't say this
if it isn't so.

But I understand that of course, you may have interpreted it as so, due to
my having failed to communicate it well.

Again, I must say, text remains replete with possibilities for
miscommunication, misunderstanding and it can so often be difficult to
communicate one's intent.

But again of course, I apologise if I overstepped the line in any way!

>
> That probably hurt most, now that a couple of hours passed. Nothing that a
> couple of beers and a bit of self-reflection on my communication style can't
> fix ;)

Ugh sorry, man. Not my intent. And it seems - I literally OWE YOU pints
now. :) we will fix this at lsf...

Perhaps owe Kalesh some too should he be there... will budget
accordingly... :P

>
> [...]
>
> > > > > > Yeah that's a good point, but honestly if you're reading smaps that reads
> > > > > > the page tables, then reading /proc/$pid/pagemaps and reading page tables
> > > > > > TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> > > > > > /proc/$pid/pagemaps and reading page tables once.
> > > > >
> > > > > Right; I recently wished that we would have an interface to obtain more VMA
> > > > > flags without having to go through smaps
> > > >
> > > > Well maybe that lends itself to the idea of adding a whole new interface in
> > > > general...
> > >
> > > An extended "maps" interface might be reasonable, that allows for exposing
> > > more things without walking the page tables. (e.g., flags)
> > >
> > > Maybe one could have an indicator that says "ever had guard regions in this
> > > mapping" without actually walking the page tables.
> >
> > Yeah this is something we've discussed before, but it's a little fraught. Let's
> > say it was a VMA flag, in this case we'd have to make this flag 'sticky' and not
> > impact merging (easy enough) to account for splits/merges.
> > > The problem comes in that we would then need to acquire the VMA write
> lock to do
> > it, something we don't currently require on application of guard regions.
>
> Right, and we shouldn't write-lock the mmap. We'd need some way to just
> atomically set such an indicator on a VMA.

Hm yeah, could be tricky, we definitely can't manage a new field in
vm_area_struct, this is a very sensitive subject at the moment really with
Suren's work with VMAs allocated via SLAB_TYPESAFE_BY_RCU, putting the lock
into the VMA and the alignment requirements.

Not sure what precedent we'd have with atomic setting of a VMA flag for
this... could be tricky.

>
> I'll also note that it might be helpful for smallish region, but especially
> for large ones (including when they are split and the indicator is wrong),
> it's less helpful. I don't have to tell you about the VMA merging
> implications, probably it would be like VM_SOFTDIRTY handling :)

Yeah indeed now we've simplified merging a lot of possibilities emerge,
this is one!

>
> >
> > We'd also have to make sure nothing else makes any assumptions about VMA flags
> > implying differences in VMAs in this one instance (though we do already do this
> > for VM_SOFTDIRTY).
> >
> > I saw this as possibly something like VM_MAYBE_GUARD_REGIONS or something.
>
> Yes.
>
> --
> Cheers,
>
> David / dhildenb
>

Best, Lorenzo

Powered by blists - more mailing lists