Messages in this thread | | | From | Joel Fernandes <> | Date | Fri, 19 May 2023 18:52:28 -0400 | Subject | Re: [PATCH v2 1/4] mm/mremap: Optimize the start addresses in move_page_tables() |
| |
Hi Linus,
On Fri, May 19, 2023 at 3:21 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, May 19, 2023 at 12:09 PM Joel Fernandes (Google) > <joel@joelfernandes.org> wrote: > > > > +static bool check_addr_in_prev(struct vm_area_struct *vma, unsigned long addr, > > + unsigned long mask) > > +{ > > + int addr_masked = addr & mask; > > + struct vm_area_struct *prev = NULL, *cur = NULL; > > + > > + /* If the masked address is within vma, there is no prev mapping of concern. */ > > + if (vma->vm_start <= addr_masked) > > + return false; > > Hmm. > > I should have caught this last time, but I didn't. > > That test smells bad to me. Or maybe it's just the comment. > > I *suspect* that the test is literally just for the stack movement > case by execve, where it catches the case where we're doing the > movement entirely within the one vma we set up.
Yes that's right, the test is only for the stack movement case. For the regular mremap case, I don't think there is a way for it to trigger.
> But in the *general* case I think the above is horribly wrong: if you > want to move pages within an existing mapping, the page moving code > can't just randomly say "I'll expand the area you wanted to move". > Again, in that general mremap() case (as opposed to the special stack > moving case for execve), I *think* that the caller has already split > the vma's at the point of the move, and this test simply cannot ever > trigger.
Yes, the test simply cannot ever trigger for mremap() but we still need the test for the stack case. That's why I had considered adding it and I had indeed reviewed the stack case when adding the test. I could update the comment to mention that, if you want.
> So I think the _code_ works, but I think the comment in particular is > questionable, and I'm a bit surprised about the code too,. because I > thought execve() only expanded to exactly the moving area.
It expands to cover both the new start and the old end of the stack AFAICS: /* * cover the whole range: [new_start, old_end) */ if (vma_expand(&vmi, vma, new_start, old_end, vma->vm_pgoff, NULL)) return -ENOMEM;
In this case, it will trigger for the stack because this same expanded vma is passed as both the new vma and the old vma to move_page_tables().
*/ if (length != move_page_tables(vma, old_start, vma, new_start, length, false)) return -ENOMEM;
So AFAICS, it is possible that old_start will start later than this newly expanded VMA. So for such a situation, old_start can be realigned to PMD and the test allows that by saying it need not worry about aligning down to an existing VMA.
> End result: I think the patch on the whole looks nice, and smaller > than I expected. I also suspect it works in practice, but I'd like > that test clarified. Does it *actually* trigger for the stack moving > case? Because I think it must (never* trigger for the mremap case?
You are right that the test will not trigger for the mremap case. But from a correctness standpoint, I thought of leaving it there for the stack (and who knows for what other future reasons the test may be needed). I can update the comment describing the stack if you like.
> And maybe I'm the one confused here, and all I really need is an > explanation with small words and simple grammar starting with "No, > Linus, this is for case xyz"
Hopefully it is clear now and you agree. Let me know if you want me to do something more. I can make some time next week to trace the stack case a bit more if you like and report back on any behaviors, however the mremap tests I did are looking good and working as expected.
thanks,
- Joel
| |