| Message ID | 3b04eb13b499df3ebf50ae3cde9a7ed5e76237fd.1722849860.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: remove vma_merge() | expand |
On Mon, 5 Aug 2024 13:13:55 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > Pull this operation into its own function and have vma_expand() call > commit_merge() instead. > > This lays the groundwork for a subsequent patch which replaces vma_merge() > with a simpler function which can share the same code. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 45 insertions(+), 12 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index a404cf718f9e..b7e3c64d5d68 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > } > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > +/* Actually perform the VMA merge operation. */ > +static int commit_merge(struct vma_merge_struct *vmg, > + struct vm_area_struct *adjust, > + struct vm_area_struct *remove, > + struct vm_area_struct *remove2, > + long adj_start, > + bool expanded) > +{ > + struct vma_prepare vp; > + > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > + > + if (expanded) { > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > + } else { > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > + adjust->vm_end); > + } It's hard to follow the logic if you the "expanded" parameter is always true. I have to look at PATCH 09/10 first to see how it is expected to be used. Is there no other way? Note that this is not needed for adjust and adj_start, because they are merely moved here from vma_expand() and passed down as parameters to other functions. Petr T > + > + if (vma_iter_prealloc(vmg->vmi, vmg->vma)) > + return -ENOMEM; > + > + vma_prepare(&vp); > + vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start); > + vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff); > + > + if (expanded) > + vma_iter_store(vmg->vmi, vmg->vma); > + > + if (adj_start) { > + adjust->vm_start += adj_start; > + adjust->vm_pgoff += PHYS_PFN(adj_start); > + if (adj_start < 0) { > + WARN_ON(expanded); > + vma_iter_store(vmg->vmi, adjust); > + } > + } > + > + vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm); > + > + return 0; > +} > + > /* > * vma_merge_new_vma - Attempt to merge a new VMA into address space > * > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg) > bool remove_next = false; > struct vm_area_struct *vma = vmg->vma; > struct vm_area_struct *next = vmg->next; > - struct vma_prepare vp; > > vma_start_write(vma); > if (next && (vma != next) && (vmg->end == next->vm_end)) { > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg) > return ret; > } > > - init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL); > /* Not merging but overwriting any part of next is not handled. */ > - VM_WARN_ON(next && !vp.remove && > + VM_WARN_ON(next && !remove_next && > next != vma && vmg->end > next->vm_start); > /* Only handles expanding */ > VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end); > > - /* Note: vma iterator must be pointing to 'start' */ > - vma_iter_config(vmg->vmi, vmg->start, vmg->end); > - if (vma_iter_prealloc(vmg->vmi, vma)) > + if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) > goto nomem; > > - vma_prepare(&vp); > - vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0); > - vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff); > - vma_iter_store(vmg->vmi, vma); > - > - vma_complete(&vp, vmg->vmi, vma->vm_mm); > return 0; > > nomem:
On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesařík wrote: > On Mon, 5 Aug 2024 13:13:55 +0100 > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > Pull this operation into its own function and have vma_expand() call > > commit_merge() instead. > > > > This lays the groundwork for a subsequent patch which replaces vma_merge() > > with a simpler function which can share the same code. > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > --- > > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 45 insertions(+), 12 deletions(-) > > > > diff --git a/mm/vma.c b/mm/vma.c > > index a404cf718f9e..b7e3c64d5d68 100644 > > --- a/mm/vma.c > > +++ b/mm/vma.c > > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > > } > > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > > > +/* Actually perform the VMA merge operation. */ > > +static int commit_merge(struct vma_merge_struct *vmg, > > + struct vm_area_struct *adjust, > > + struct vm_area_struct *remove, > > + struct vm_area_struct *remove2, > > + long adj_start, > > + bool expanded) > > +{ > > + struct vma_prepare vp; > > + > > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > + > > + if (expanded) { > > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > + } else { > > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > > + adjust->vm_end); > > + } > > It's hard to follow the logic if you the "expanded" parameter is always > true. I have to look at PATCH 09/10 first to see how it is expected to > be used. Is there no other way? > > Note that this is not needed for adjust and adj_start, because they are > merely moved here from vma_expand() and passed down as parameters to > other functions. See the next patch to understand how these are used, as the commit message says, this lays the groundwork for the next patch which actually uses both of these. I have tried hard to clarify how these are used, however there is some unavoidable and inherent complexity in this logic. If you don't believe me, I suggest trying to follow the logic of the existing code :) And if you want to _really_ have fun, I suggest you try to understand the logic around v6.0 prior to Liam's interventions. We might be able to try to improve the logic flow further, but it's one step at a time with this. > > Petr T > > > + > > + if (vma_iter_prealloc(vmg->vmi, vmg->vma)) > > + return -ENOMEM; > > + > > + vma_prepare(&vp); > > + vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start); > > + vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff); > > + > > + if (expanded) > > + vma_iter_store(vmg->vmi, vmg->vma); > > + > > + if (adj_start) { > > + adjust->vm_start += adj_start; > > + adjust->vm_pgoff += PHYS_PFN(adj_start); > > + if (adj_start < 0) { > > + WARN_ON(expanded); > > + vma_iter_store(vmg->vmi, adjust); > > + } > > + } > > + > > + vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm); > > + > > + return 0; > > +} > > + > > /* > > * vma_merge_new_vma - Attempt to merge a new VMA into address space > > * > > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg) > > bool remove_next = false; > > struct vm_area_struct *vma = vmg->vma; > > struct vm_area_struct *next = vmg->next; > > - struct vma_prepare vp; > > > > vma_start_write(vma); > > if (next && (vma != next) && (vmg->end == next->vm_end)) { > > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg) > > return ret; > > } > > > > - init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL); > > /* Not merging but overwriting any part of next is not handled. */ > > - VM_WARN_ON(next && !vp.remove && > > + VM_WARN_ON(next && !remove_next && > > next != vma && vmg->end > next->vm_start); > > /* Only handles expanding */ > > VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end); > > > > - /* Note: vma iterator must be pointing to 'start' */ > > - vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > - if (vma_iter_prealloc(vmg->vmi, vma)) > > + if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) > > goto nomem; > > > > - vma_prepare(&vp); > > - vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0); > > - vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff); > > - vma_iter_store(vmg->vmi, vma); > > - > > - vma_complete(&vp, vmg->vmi, vma->vm_mm); > > return 0; > > > > nomem: >
On Tue, 6 Aug 2024 14:48:33 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesařík wrote: > > On Mon, 5 Aug 2024 13:13:55 +0100 > > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > Pull this operation into its own function and have vma_expand() call > > > commit_merge() instead. > > > > > > This lays the groundwork for a subsequent patch which replaces vma_merge() > > > with a simpler function which can share the same code. > > > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > --- > > > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > > > 1 file changed, 45 insertions(+), 12 deletions(-) > > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > index a404cf718f9e..b7e3c64d5d68 100644 > > > --- a/mm/vma.c > > > +++ b/mm/vma.c > > > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > > > } > > > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > > > > > +/* Actually perform the VMA merge operation. */ > > > +static int commit_merge(struct vma_merge_struct *vmg, > > > + struct vm_area_struct *adjust, > > > + struct vm_area_struct *remove, > > > + struct vm_area_struct *remove2, > > > + long adj_start, > > > + bool expanded) > > > +{ > > > + struct vma_prepare vp; > > > + > > > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > > + > > > + if (expanded) { > > > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > > + } else { > > > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > > > + adjust->vm_end); > > > + } > > > > It's hard to follow the logic if you the "expanded" parameter is always > > true. I have to look at PATCH 09/10 first to see how it is expected to > > be used. Is there no other way? > > > > Note that this is not needed for adjust and adj_start, because they are > > merely moved here from vma_expand() and passed down as parameters to > > other functions. > > See the next patch to understand how these are used, as the commit message > says, this lays the groundwork for the next patch which actually uses both > of these. > > I have tried hard to clarify how these are used, however there is some > unavoidable and inherent complexity in this logic. If you don't believe me, > I suggest trying to follow the logic of the existing code :) > > And if you want to _really_ have fun, I suggest you try to understand the > logic around v6.0 prior to Liam's interventions. > > We might be able to try to improve the logic flow further, but it's one > step at a time with this. What I mean is: Is there no way to arrange the patch series so that I don't have to look at PATH 09/10 before I can understand code in patch 08/10? This PATCH 08/10 adds only one call to commit_merge() and that one always sets expanded to true. Maybe you could introduce commit_merge() here without the parameter and add it in PATCH 09/10? Petr T
On Tue, Aug 06, 2024 at 04:13:21PM GMT, Petr Tesařík wrote: > On Tue, 6 Aug 2024 14:48:33 +0100 > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesařík wrote: > > > On Mon, 5 Aug 2024 13:13:55 +0100 > > > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > > > Pull this operation into its own function and have vma_expand() call > > > > commit_merge() instead. > > > > > > > > This lays the groundwork for a subsequent patch which replaces vma_merge() > > > > with a simpler function which can share the same code. > > > > > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > > --- > > > > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > > > > 1 file changed, 45 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > > index a404cf718f9e..b7e3c64d5d68 100644 > > > > --- a/mm/vma.c > > > > +++ b/mm/vma.c > > > > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > > > > } > > > > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > > > > > > > +/* Actually perform the VMA merge operation. */ > > > > +static int commit_merge(struct vma_merge_struct *vmg, > > > > + struct vm_area_struct *adjust, > > > > + struct vm_area_struct *remove, > > > > + struct vm_area_struct *remove2, > > > > + long adj_start, > > > > + bool expanded) > > > > +{ > > > > + struct vma_prepare vp; > > > > + > > > > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > > > + > > > > + if (expanded) { > > > > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > > > + } else { > > > > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > > > > + adjust->vm_end); > > > > + } > > > > > > It's hard to follow the logic if you the "expanded" parameter is always > > > true. I have to look at PATCH 09/10 first to see how it is expected to > > > be used. Is there no other way? > > > > > > Note that this is not needed for adjust and adj_start, because they are > > > merely moved here from vma_expand() and passed down as parameters to > > > other functions. > > > > See the next patch to understand how these are used, as the commit message > > says, this lays the groundwork for the next patch which actually uses both > > of these. > > > > I have tried hard to clarify how these are used, however there is some > > unavoidable and inherent complexity in this logic. If you don't believe me, > > I suggest trying to follow the logic of the existing code :) > > > > And if you want to _really_ have fun, I suggest you try to understand the > > logic around v6.0 prior to Liam's interventions. > > > > We might be able to try to improve the logic flow further, but it's one > > step at a time with this. > > What I mean is: Is there no way to arrange the patch series so that I > don't have to look at PATH 09/10 before I can understand code in patch > 08/10? No. > > This PATCH 08/10 adds only one call to commit_merge() and that one > always sets expanded to true. Maybe you could introduce commit_merge() > here without the parameter and add it in PATCH 09/10? No, I won't do that, you haven't made a case for it. > > Petr T I appreciate you are doing a drive-by review on code you aren't familiar with, but it's worth appreciating that there is some context here - this is intentionally isolating _existing_ logic from vma_expand() and vma_merge() in such a way that we have a _generic_ function we can use for this operation. I think it'd be _more_ confusing and (surprising given your rather pedantic interpretation of churn elsewhere) churny to rewrite this again with a bunch of added logic in the next commit. I think this is highly subjective, and I'm not sure it's a great use of either of our time to get too stuck in the weeds on this kind of thing. Of course if you or others can present a more compelling argument for reworking this I'm happy to hear.
On Tue, 6 Aug 2024 15:30:49 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > On Tue, Aug 06, 2024 at 04:13:21PM GMT, Petr Tesařík wrote: > > On Tue, 6 Aug 2024 14:48:33 +0100 > > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesařík wrote: > > > > On Mon, 5 Aug 2024 13:13:55 +0100 > > > > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > Pull this operation into its own function and have vma_expand() call > > > > > commit_merge() instead. > > > > > > > > > > This lays the groundwork for a subsequent patch which replaces vma_merge() > > > > > with a simpler function which can share the same code. > > > > > > > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > > > --- > > > > > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > > > > > 1 file changed, 45 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > > > index a404cf718f9e..b7e3c64d5d68 100644 > > > > > --- a/mm/vma.c > > > > > +++ b/mm/vma.c > > > > > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > > > > > } > > > > > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > > > > > > > > > +/* Actually perform the VMA merge operation. */ > > > > > +static int commit_merge(struct vma_merge_struct *vmg, > > > > > + struct vm_area_struct *adjust, > > > > > + struct vm_area_struct *remove, > > > > > + struct vm_area_struct *remove2, > > > > > + long adj_start, > > > > > + bool expanded) > > > > > +{ > > > > > + struct vma_prepare vp; > > > > > + > > > > > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > > > > + > > > > > + if (expanded) { > > > > > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > > > > + } else { > > > > > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > > > > > + adjust->vm_end); > > > > > + } > > > > > > > > It's hard to follow the logic if you the "expanded" parameter is always > > > > true. I have to look at PATCH 09/10 first to see how it is expected to > > > > be used. Is there no other way? > > > > > > > > Note that this is not needed for adjust and adj_start, because they are > > > > merely moved here from vma_expand() and passed down as parameters to > > > > other functions. > > > > > > See the next patch to understand how these are used, as the commit message > > > says, this lays the groundwork for the next patch which actually uses both > > > of these. > > > > > > I have tried hard to clarify how these are used, however there is some > > > unavoidable and inherent complexity in this logic. If you don't believe me, > > > I suggest trying to follow the logic of the existing code :) > > > > > > And if you want to _really_ have fun, I suggest you try to understand the > > > logic around v6.0 prior to Liam's interventions. > > > > > > We might be able to try to improve the logic flow further, but it's one > > > step at a time with this. > > > > What I mean is: Is there no way to arrange the patch series so that I > > don't have to look at PATH 09/10 before I can understand code in patch > > 08/10? > > No. > > > > > This PATCH 08/10 adds only one call to commit_merge() and that one > > always sets expanded to true. Maybe you could introduce commit_merge() > > here without the parameter and add it in PATCH 09/10? > > No, I won't do that, you haven't made a case for it. > > > > > Petr T > > I appreciate you are doing a drive-by review on code you aren't familiar > with, but it's worth appreciating that there is some context here - this is > intentionally isolating _existing_ logic from vma_expand() and vma_merge() > in such a way that we have a _generic_ function we can use for this > operation. The history you make today becomes the learning material for the next generation of kernel hackers (who will also lack a lot of context). > I think it'd be _more_ confusing and (surprising given your rather pedantic > interpretation of churn elsewhere) churny to rewrite this again with a > bunch of added logic in the next commit. > > I think this is highly subjective, and I'm not sure it's a great use of > either of our time to get too stuck in the weeds on this kind of thing. Yep. We can all agre this is the best way to convey the idea behind the changes. Don't get me wrong; this whole series does a lot of good in terms of code readability, even for a bystander like myself. Petr T
On 8/5/24 14:13, Lorenzo Stoakes wrote: > Pull this operation into its own function and have vma_expand() call > commit_merge() instead. > > This lays the groundwork for a subsequent patch which replaces vma_merge() > with a simpler function which can share the same code. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> In general, Acked-by: Vlastimil Babka <vbabka@suse.cz> If you consider the following suggestions, great: > --- > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 45 insertions(+), 12 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index a404cf718f9e..b7e3c64d5d68 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > } > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > +/* Actually perform the VMA merge operation. */ > +static int commit_merge(struct vma_merge_struct *vmg, > + struct vm_area_struct *adjust, > + struct vm_area_struct *remove, > + struct vm_area_struct *remove2, > + long adj_start, > + bool expanded) I've read the subthread with Petr. I understand it's hard to organize such big changes in self-contained units. But maybe it would still be possible to introduce this function now without the parameters, and as part of the the next patch add the two parameters and the code using them. Maybe it would even make git detect the added code as code move from where it's now, so it would be more obviousl. > +{ > + struct vma_prepare vp; > + > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > + > + if (expanded) { > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); This originally had a comment /* Note: vma iterator must be pointing to 'start' */ and now it's gone. > + } else { > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > + adjust->vm_end); And this less obvious one has none either :( > + } > + > + if (vma_iter_prealloc(vmg->vmi, vmg->vma)) > + return -ENOMEM; > + > + vma_prepare(&vp); > + vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start); > + vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff); > + > + if (expanded) > + vma_iter_store(vmg->vmi, vmg->vma); > + > + if (adj_start) { > + adjust->vm_start += adj_start; > + adjust->vm_pgoff += PHYS_PFN(adj_start); > + if (adj_start < 0) { > + WARN_ON(expanded); > + vma_iter_store(vmg->vmi, adjust); > + } > + } > + > + vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm); > + > + return 0; > +} > + > /* > * vma_merge_new_vma - Attempt to merge a new VMA into address space > * > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg) > bool remove_next = false; > struct vm_area_struct *vma = vmg->vma; > struct vm_area_struct *next = vmg->next; > - struct vma_prepare vp; > > vma_start_write(vma); > if (next && (vma != next) && (vmg->end == next->vm_end)) { > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg) > return ret; > } > > - init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL); > /* Not merging but overwriting any part of next is not handled. */ > - VM_WARN_ON(next && !vp.remove && > + VM_WARN_ON(next && !remove_next && > next != vma && vmg->end > next->vm_start); > /* Only handles expanding */ > VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end); > > - /* Note: vma iterator must be pointing to 'start' */ > - vma_iter_config(vmg->vmi, vmg->start, vmg->end); > - if (vma_iter_prealloc(vmg->vmi, vma)) > + if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) > goto nomem; > > - vma_prepare(&vp); > - vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0); > - vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff); > - vma_iter_store(vmg->vmi, vma); > - > - vma_complete(&vp, vmg->vmi, vma->vm_mm); > return 0; > > nomem:
On Fri, Aug 09, 2024 at 12:15:24PM GMT, Vlastimil Babka wrote: > On 8/5/24 14:13, Lorenzo Stoakes wrote: > > Pull this operation into its own function and have vma_expand() call > > commit_merge() instead. > > > > This lays the groundwork for a subsequent patch which replaces vma_merge() > > with a simpler function which can share the same code. > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > In general, > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > > If you consider the following suggestions, great: > > > --- > > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 45 insertions(+), 12 deletions(-) > > > > diff --git a/mm/vma.c b/mm/vma.c > > index a404cf718f9e..b7e3c64d5d68 100644 > > --- a/mm/vma.c > > +++ b/mm/vma.c > > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > > } > > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > > > +/* Actually perform the VMA merge operation. */ > > +static int commit_merge(struct vma_merge_struct *vmg, > > + struct vm_area_struct *adjust, > > + struct vm_area_struct *remove, > > + struct vm_area_struct *remove2, > > + long adj_start, > > + bool expanded) > > I've read the subthread with Petr. I understand it's hard to organize such > big changes in self-contained units. But maybe it would still be possible to > introduce this function now without the parameters, and as part of the the > next patch add the two parameters and the code using them. Maybe it would > even make git detect the added code as code move from where it's now, so it > would be more obviousl. Since both you and Petr make the same point (sorry Petr, I should have perhaps been a little less resistant to this), I will do this. As discussed on IRC my position on this is that we're introducing a _really fundamental_ and important bit of the logic here, intentionally broken out as a separate commit, and this is why I preferred to introduce it 'fully formed'. This function is absolutely fundamental to eliminating the duplication in do_brk_flags() + mmap_region() and to maintain two separate new/modified vma versions of vma_merge(). HOWEVER, I totally accept that this makes review much more of a pain in the arse, and in practice almost certainly the only thing that matters is reviewability here as to how I structure this. So TL;DR: I'll do what you both ask and introduce new params only when we use them. > > > +{ > > + struct vma_prepare vp; > > + > > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > + > > + if (expanded) { > > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > This originally had a comment > > /* Note: vma iterator must be pointing to 'start' */ > > and now it's gone. Will check and re-add if it makes sense. I mean we're now setting the iterator to start anyway so I don't see that this has value? Maybe I'm missing something and Liam has thoughts... > > > + } else { > > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > > + adjust->vm_end); > > And this less obvious one has none either :( I will add a comment. > > > + } > > + > > + if (vma_iter_prealloc(vmg->vmi, vmg->vma)) > > + return -ENOMEM; > > + > > + vma_prepare(&vp); > > + vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start); > > + vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff); > > + > > + if (expanded) > > + vma_iter_store(vmg->vmi, vmg->vma); > > + > > + if (adj_start) { > > + adjust->vm_start += adj_start; > > + adjust->vm_pgoff += PHYS_PFN(adj_start); > > + if (adj_start < 0) { > > + WARN_ON(expanded); > > + vma_iter_store(vmg->vmi, adjust); > > + } > > + } > > + > > + vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm); > > + > > + return 0; > > +} > > + > > /* > > * vma_merge_new_vma - Attempt to merge a new VMA into address space > > * > > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg) > > bool remove_next = false; > > struct vm_area_struct *vma = vmg->vma; > > struct vm_area_struct *next = vmg->next; > > - struct vma_prepare vp; > > > > vma_start_write(vma); > > if (next && (vma != next) && (vmg->end == next->vm_end)) { > > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg) > > return ret; > > } > > > > - init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL); > > /* Not merging but overwriting any part of next is not handled. */ > > - VM_WARN_ON(next && !vp.remove && > > + VM_WARN_ON(next && !remove_next && > > next != vma && vmg->end > next->vm_start); > > /* Only handles expanding */ > > VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end); > > > > - /* Note: vma iterator must be pointing to 'start' */ > > - vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > - if (vma_iter_prealloc(vmg->vmi, vma)) > > + if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) > > goto nomem; > > > > - vma_prepare(&vp); > > - vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0); > > - vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff); > > - vma_iter_store(vmg->vmi, vma); > > - > > - vma_complete(&vp, vmg->vmi, vma->vm_mm); > > return 0; > > > > nomem: >
diff --git a/mm/vma.c b/mm/vma.c index a404cf718f9e..b7e3c64d5d68 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) } #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ +/* Actually perform the VMA merge operation. */ +static int commit_merge(struct vma_merge_struct *vmg, + struct vm_area_struct *adjust, + struct vm_area_struct *remove, + struct vm_area_struct *remove2, + long adj_start, + bool expanded) +{ + struct vma_prepare vp; + + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); + + if (expanded) { + vma_iter_config(vmg->vmi, vmg->start, vmg->end); + } else { + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, + adjust->vm_end); + } + + if (vma_iter_prealloc(vmg->vmi, vmg->vma)) + return -ENOMEM; + + vma_prepare(&vp); + vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start); + vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff); + + if (expanded) + vma_iter_store(vmg->vmi, vmg->vma); + + if (adj_start) { + adjust->vm_start += adj_start; + adjust->vm_pgoff += PHYS_PFN(adj_start); + if (adj_start < 0) { + WARN_ON(expanded); + vma_iter_store(vmg->vmi, adjust); + } + } + + vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm); + + return 0; +} + /* * vma_merge_new_vma - Attempt to merge a new VMA into address space * @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg) bool remove_next = false; struct vm_area_struct *vma = vmg->vma; struct vm_area_struct *next = vmg->next; - struct vma_prepare vp; vma_start_write(vma); if (next && (vma != next) && (vmg->end == next->vm_end)) { @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg) return ret; } - init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL); /* Not merging but overwriting any part of next is not handled. */ - VM_WARN_ON(next && !vp.remove && + VM_WARN_ON(next && !remove_next && next != vma && vmg->end > next->vm_start); /* Only handles expanding */ VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end); - /* Note: vma iterator must be pointing to 'start' */ - vma_iter_config(vmg->vmi, vmg->start, vmg->end); - if (vma_iter_prealloc(vmg->vmi, vma)) + if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) goto nomem; - vma_prepare(&vp); - vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0); - vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff); - vma_iter_store(vmg->vmi, vma); - - vma_complete(&vp, vmg->vmi, vma->vm_mm); return 0; nomem:
Pull this operation into its own function and have vma_expand() call commit_merge() instead. This lays the groundwork for a subsequent patch which replaces vma_merge() with a simpler function which can share the same code. Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 12 deletions(-)