| Message ID | 20250127075527.16614-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vma: fix unmapped_area() | expand |
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. 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. 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. 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. > > 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 <richard.weiyang@gmail.com> [250127 02:55]: > The gap check in unmapped_area() seems not correct. > This is not an acceptable cover letter. You must have spent a significant amount of time trying to figure it out and spent about 2 seconds on explaining what you think is going on here. I'm not convinced there's an issue here at all, and I'm not going to spend time on it after this cover letter. It seems like you are also not sure there's a problem. If it's important enough to fix then it's important enough to explain what isn't correct. If it's not important enough to explain then don't bother fixing it. Thanks, Liam
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. 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 >> >>
On Mon, Jan 27, 2025 at 09:19:24AM -0500, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [250127 02:55]: >> The gap check in unmapped_area() seems not correct. >> > >This is not an acceptable cover letter. > >You must have spent a significant amount of time trying to figure it out >and spent about 2 seconds on explaining what you think is going on here. > Sometimes I am not sure what should be put into the cover letter. Will adjust next time. >I'm not convinced there's an issue here at all, and I'm not going to >spend time on it after this cover letter. It seems like you are also >not sure there's a problem. > >If it's important enough to fix then it's important enough to explain >what isn't correct. > >If it's not important enough to explain then don't bother fixing it. > >Thanks, >Liam
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 >