Re: [PATCH 0/2] vma: fix unmapped_area()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Jan 28, 2025 at 01:56:19AM +0000, Wei Yang wrote:
> On Mon, Jan 27, 2025 at 10:50:17AM +0000, Lorenzo Stoakes wrote:
> >Hi Wei,
> >
> >I seem to recall us having a very recent converation about holding off on
> >patches like these for a little while to which you agreed, and then you
> >sent this pretty much the very next day? And during the merge window?
> >Honestly not _hugely_ impressed with that.
> >
>
> Yes I remember your suggestion. I send this because it is a bug fix to me.
> Per my understanding on your word, it is ok to send a fix.

This is not a bug. A bug is something that breaks the kernel.

Having to use a different gap in the vastness of virtual address space is
not a bug.

This is you misunderstanding unmapped_area() as a function that for some
reason in your view must function identically in being minimally
conservative whether top-down or bottom-up including scenarios in which
start_gap is an impossible value.

As far as I can tell the function is behaving completely correctly, it is
just conservative in accounting for worst-case alignment and front and rear
gaps of appropriate size.

You could have more civilly tested out this theory, rather than wasting
presumably a LOT of your time on this (meanwhile taking -no- time to write
commit messages), simply asking Liam about it on-list.

Communication is key.

>
> If I misunderstand, I apologize.
>
> >In my view this patch should have instead started as a query to Liam about
> >the gap calculation, this would have been far more civil and would have
> >allowed us to determine for sure if the approach you've taken here is
> >valid.
> >
>
> You are right. I will try to be better next time.
>
> As you mentioned a query before sending a patch, this is preferred, right?
> Hope I don't mess this again.
>
> >Given your history of sending entirely trivial patches which we keep asking
> >you not to send (mixed in with the occasional actually useful patch) it is
> >KEY to communicate to ensure we're on the same page.
> >
> >If you send meaningful commits, we want to merge them. Arbitrarily sending
> >something like this, at this point in time, when you've been asked not to -
> >does not help achieve this aim.
> >
>
> Thanks, I would be more considerate next time.
>
> >On Mon, Jan 27, 2025 at 07:55:25AM +0000, Wei Yang wrote:
> >> The gap check in unmapped_area() seems not correct.
> >>
> >> Add test cases to verify the behavior.
> >
> >This is an -entirely- unacceptable cover letter. It's two lines dude. Give
> >some details. You're actually tackling a very, very specific aspect and
> >scenario in some of the most sensitive code in all of mm.
> >
> >You really, really need to be clear on what it is you're doing, why, what
> >workload you were doing to hit this, what testing you've done, what real
> >life things this interacts with etc. etc.
> >
> >It makes our lives easier as maintainers. Right now I see this as 'another
> >trivial Wei patch', you need to provide details to prove otherwise, if that
> >is indeed, not the case.
> >
> >Also your subject line here is horrible - 'fix unmapped_area()' - actually
> >you seem to be (in your view) correcting the calculation with respect to
> >upward-growing stacks. Correct me if I'm wrong. I mean even your patch 1/2
> >has a better message... It needs to be more specific to what you're doing.
> >
>
> Thanks to you and Liam. I will try to do better to not waste your time.
>
> >>
> >> Wei Yang (2):
> >>   mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN
> >>   tools: testing: add unmapped_area() tests
> >>
> >>  mm/vma.c                         |   2 +-
> >>  tools/testing/vma/vma.c          | 177 +++++++++++++++++++++++++++++++
> >>  tools/testing/vma/vma_internal.h |   2 +-
> >>  3 files changed, 179 insertions(+), 2 deletions(-)
> >>
> >> --
> >> 2.34.1
> >>
> >>
>
> --
> Wei Yang
> Help you, Help me
>