| Message ID | 20241025031847.6274-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vma: miss to restore vmi.index on expansion failure | expand |
On Fri, Oct 25, 2024 at 03:18:45AM +0000, Wei Yang wrote: > On expansion failure, we try to restore vmg state, but we missed to > restore vmi.index. The reason is we have reset vmg->vma before checking. > So let's put the operation before reset vmg->vma. > > Also we don't need to do the restore if there is no mergeable adjacent > VMA. Let's take it out to skip the unnecessary operations. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/vma.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index fb4f1863f88e..c94d953d453c 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) > vma_prev(vmg->vmi); /* Equivalent to going to the previous range */ > } > > + /* No mergeable adjacent VMA, return */ > + if (!vmg->vma) > + return NULL; > + Kind of a pet peeve of mine is throwing in a random refactoring thats not mentioned in the commit message. Please don't do that. I think it's fine as it is. > /* > * Now try to expand adjacent VMA(s). This takes care of removing the > * following VMA if we have VMAs on both sides. > */ > - if (vmg->vma && !vma_expand(vmg)) { > + if (!vma_expand(vmg)) { > khugepaged_enter_vma(vmg->vma, vmg->flags); > vmg->state = VMA_MERGE_SUCCESS; > return vmg->vma; > } > > /* If expansion failed, reset state. Allows us to retry merge later. */ > + if (vmg->vma == prev) > + vma_iter_set(vmg->vmi, start); Good spot in that we've stupidly been setting the vma NULL each time before comparing... (doh and mea culpa!), but this actually accidentally proves we don't need to bother resetting this at all :) The only case where we care about a reset is mmap_region(), and there we reset the iterator _anyway_. > vmg->vma = NULL; > vmg->start = start; > vmg->end = end; > vmg->pgoff = pgoff; > - if (vmg->vma == prev) > - vma_iter_set(vmg->vmi, start); So please replace this whole series with a patch that just removes these lines, thanks! Also what tree are you making this change against? All mm changes should be against akpm's tree in the mm-unstable branch. This change looks like it's against another tree, as the code for this function has changed. > > return NULL; > } > -- > 2.34.1 > >
On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote: >On Fri, Oct 25, 2024 at 03:18:45AM +0000, Wei Yang wrote: >> On expansion failure, we try to restore vmg state, but we missed to >> restore vmi.index. The reason is we have reset vmg->vma before checking. >> So let's put the operation before reset vmg->vma. >> >> Also we don't need to do the restore if there is no mergeable adjacent >> VMA. Let's take it out to skip the unnecessary operations. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> >> --- >> mm/vma.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/mm/vma.c b/mm/vma.c >> index fb4f1863f88e..c94d953d453c 100644 >> --- a/mm/vma.c >> +++ b/mm/vma.c >> @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> vma_prev(vmg->vmi); /* Equivalent to going to the previous range */ >> } >> >> + /* No mergeable adjacent VMA, return */ >> + if (!vmg->vma) >> + return NULL; >> + > >Kind of a pet peeve of mine is throwing in a random refactoring thats not >mentioned in the commit message. Please don't do that. > >I think it's fine as it is. > >> /* >> * Now try to expand adjacent VMA(s). This takes care of removing the >> * following VMA if we have VMAs on both sides. >> */ >> - if (vmg->vma && !vma_expand(vmg)) { >> + if (!vma_expand(vmg)) { >> khugepaged_enter_vma(vmg->vma, vmg->flags); >> vmg->state = VMA_MERGE_SUCCESS; >> return vmg->vma; >> } >> >> /* If expansion failed, reset state. Allows us to retry merge later. */ >> + if (vmg->vma == prev) >> + vma_iter_set(vmg->vmi, start); > >Good spot in that we've stupidly been setting the vma NULL each time before >comparing... (doh and mea culpa!), but this actually accidentally proves we >don't need to bother resetting this at all :) > >The only case where we care about a reset is mmap_region(), and there we reset >the iterator _anyway_. > >> vmg->vma = NULL; >> vmg->start = start; >> vmg->end = end; >> vmg->pgoff = pgoff; >> - if (vmg->vma == prev) >> - vma_iter_set(vmg->vmi, start); > >So please replace this whole series with a patch that just removes these >lines, thanks! You mean this? --- a/mm/vma.c +++ b/mm/vma.c @@ -964,14 +964,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) return vmg->vma; } - /* If expansion failed, reset state. Allows us to retry merge later. */ - vmg->vma = NULL; - vmg->start = start; - vmg->end = end; - vmg->pgoff = pgoff; - if (vmg->vma == prev) - vma_iter_set(vmg->vmi, start); - return NULL; } > >Also what tree are you making this change against? All mm changes should be >against akpm's tree in the mm-unstable branch. This change looks like it's >against another tree, as the code for this function has changed. > I use the upstream. Will rebase to mm-unstable next. >> >> return NULL; >> } >> -- >> 2.34.1 >> >>
On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote: >On Fri, Oct 25, 2024 at 03:18:45AM +0000, Wei Yang wrote: >> On expansion failure, we try to restore vmg state, but we missed to >> restore vmi.index. The reason is we have reset vmg->vma before checking. >> So let's put the operation before reset vmg->vma. >> >> Also we don't need to do the restore if there is no mergeable adjacent >> VMA. Let's take it out to skip the unnecessary operations. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> >> --- >> mm/vma.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/mm/vma.c b/mm/vma.c >> index fb4f1863f88e..c94d953d453c 100644 >> --- a/mm/vma.c >> +++ b/mm/vma.c >> @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> vma_prev(vmg->vmi); /* Equivalent to going to the previous range */ >> } >> >> + /* No mergeable adjacent VMA, return */ >> + if (!vmg->vma) >> + return NULL; >> + > >Kind of a pet peeve of mine is throwing in a random refactoring thats not >mentioned in the commit message. Please don't do that. > >I think it's fine as it is. > >> /* >> * Now try to expand adjacent VMA(s). This takes care of removing the >> * following VMA if we have VMAs on both sides. >> */ >> - if (vmg->vma && !vma_expand(vmg)) { >> + if (!vma_expand(vmg)) { >> khugepaged_enter_vma(vmg->vma, vmg->flags); >> vmg->state = VMA_MERGE_SUCCESS; >> return vmg->vma; >> } >> >> /* If expansion failed, reset state. Allows us to retry merge later. */ >> + if (vmg->vma == prev) >> + vma_iter_set(vmg->vmi, start); > >Good spot in that we've stupidly been setting the vma NULL each time before >comparing... (doh and mea culpa!), but this actually accidentally proves we >don't need to bother resetting this at all :) > >The only case where we care about a reset is mmap_region(), and there we reset >the iterator _anyway_. > >> vmg->vma = NULL; >> vmg->start = start; >> vmg->end = end; >> vmg->pgoff = pgoff; >> - if (vmg->vma == prev) >> - vma_iter_set(vmg->vmi, start); > >So please replace this whole series with a patch that just removes these >lines, thanks! > >Also what tree are you making this change against? All mm changes should be >against akpm's tree in the mm-unstable branch. This change looks like it's >against another tree, as the code for this function has changed. > For mm-unstable, this is what you expect? diff --git a/mm/vma.c b/mm/vma.c index b5c1adcb6992..03b4838026ab 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -1003,16 +1003,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) return vmg->vma; } - /* If expansion failed, reset state. Allows us to retry merge later. */ - if (!just_expand) { - vmg->vma = NULL; - vmg->start = start; - vmg->end = end; - vmg->pgoff = pgoff; - if (vmg->vma == prev) - vma_iter_set(vmg->vmi, start); - } - return NULL; } >> >> return NULL; >> } >> -- >> 2.34.1 >> >>
On Fri, Oct 25, 2024 at 07:59:55AM +0000, Wei Yang wrote: > On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote: > >> vmg->vma = NULL; > >> vmg->start = start; > >> vmg->end = end; > >> vmg->pgoff = pgoff; > >> - if (vmg->vma == prev) > >> - vma_iter_set(vmg->vmi, start); > > > >So please replace this whole series with a patch that just removes these > >lines, thanks! > > > >Also what tree are you making this change against? All mm changes should be > >against akpm's tree in the mm-unstable branch. This change looks like it's > >against another tree, as the code for this function has changed. > > > > For mm-unstable, this is what you expect? > > diff --git a/mm/vma.c b/mm/vma.c > index b5c1adcb6992..03b4838026ab 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -1003,16 +1003,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) > return vmg->vma; > } > > - /* If expansion failed, reset state. Allows us to retry merge later. */ > - if (!just_expand) { > - vmg->vma = NULL; > - vmg->start = start; > - vmg->end = end; > - vmg->pgoff = pgoff; > - if (vmg->vma == prev) > - vma_iter_set(vmg->vmi, start); > - } > - Noooo! Sorry I wasn't clear :) We need this. I mean: - if (vmg->vma == prev) - vma_iter_set(vmg->vmi, start); And with an explanation like: We incorrectly set vmg->vma = NULL before checking to see if we must reset the VMA iterator. However, since the only use case for this reset is mmap_region() and we always reset iterators there anyway, there is simply no need to do this. However, we absolutely do need to reset the vmg parameters to what they originally were, as well as resetting vmg->vma, as these may have been mutated to attempt a merge. There will be no change in behaviour, rather we simply avoid a pointless compare and, for cases where the VMA was the first in the mm, a pointless assignment to mas parameters. > return NULL; > } > > > >> > >> return NULL; > >> } > >> -- > >> 2.34.1 > >> > >> > > -- > Wei Yang > Help you, Help me > I want to refactor this further, but only after recent mmap_region() changes have settled down. Thanks!
On Fri, Oct 25, 2024 at 09:10:09AM +0100, Lorenzo Stoakes wrote: >On Fri, Oct 25, 2024 at 07:59:55AM +0000, Wei Yang wrote: >> On Fri, Oct 25, 2024 at 08:06:06AM +0100, Lorenzo Stoakes wrote: >> >> vmg->vma = NULL; >> >> vmg->start = start; >> >> vmg->end = end; >> >> vmg->pgoff = pgoff; >> >> - if (vmg->vma == prev) >> >> - vma_iter_set(vmg->vmi, start); >> > >> >So please replace this whole series with a patch that just removes these >> >lines, thanks! >> > >> >Also what tree are you making this change against? All mm changes should be >> >against akpm's tree in the mm-unstable branch. This change looks like it's >> >against another tree, as the code for this function has changed. >> > >> >> For mm-unstable, this is what you expect? >> >> diff --git a/mm/vma.c b/mm/vma.c >> index b5c1adcb6992..03b4838026ab 100644 >> --- a/mm/vma.c >> +++ b/mm/vma.c >> @@ -1003,16 +1003,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> return vmg->vma; >> } >> >> - /* If expansion failed, reset state. Allows us to retry merge later. */ >> - if (!just_expand) { >> - vmg->vma = NULL; >> - vmg->start = start; >> - vmg->end = end; >> - vmg->pgoff = pgoff; >> - if (vmg->vma == prev) >> - vma_iter_set(vmg->vmi, start); >> - } >> - > >Noooo! Sorry I wasn't clear :) We need this. > >I mean: > > - if (vmg->vma == prev) > - vma_iter_set(vmg->vmi, start); > >And with an explanation like: > > We incorrectly set vmg->vma = NULL before checking to see if we > must reset the VMA iterator. However, since the only use case for > this reset is mmap_region() and we always reset iterators there > anyway, there is simply no need to do this. > > However, we absolutely do need to reset the vmg parameters to what > they originally were, as well as resetting vmg->vma, as these may > have been mutated to attempt a merge. > > There will be no change in behaviour, rather we simply avoid a > pointless compare and, for cases where the VMA was the first in the > mm, a pointless assignment to mas parameters. > Ok, I get what you want. But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. Lets say just_expand is true and can_merge_left is true. Now we will adjust vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't restore vmg->vma/start/pgoff, since just_expand is true. Is this what you expect? >> return NULL; >> } >> >> >> >> >> >> return NULL; >> >> } >> >> -- >> >> 2.34.1 >> >> >> >> >> >> -- >> Wei Yang >> Help you, Help me >> > >I want to refactor this further, but only after recent mmap_region() >changes have settled down. > >Thanks!
On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote: > > But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. > > Lets say just_expand is true and can_merge_left is true. Now we will adjust > vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't > restore vmg->vma/start/pgoff, since just_expand is true. > > Is this what you expect? Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't realise :) Actually at this point, I don't think we need a follow up patch, sorry. As I think perhaps I will make this change as part of an existing series where I am reworking mmap_region(), since this is the only place where it matters, and it would make everything a hell of a lot clearer. Thanks for pointing this out, it's very useful (and an embarrassing oversight on my part...!), but I think it'd be better reworked this way. Thanks!
On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote: >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote: > >> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. >> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't >> restore vmg->vma/start/pgoff, since just_expand is true. >> >> Is this what you expect? > >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't >realise :) > >Actually at this point, I don't think we need a follow up patch, sorry. > >As I think perhaps I will make this change as part of an existing series >where I am reworking mmap_region(), since this is the only place where it >matters, and it would make everything a hell of a lot clearer. > >Thanks for pointing this out, it's very useful (and an embarrassing >oversight on my part...!), but I think it'd be better reworked this way. > >Thanks! Ok, for now I would just remove these two lines with the change log you suggested.
On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote: > On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote: > >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote: > > > >> > >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. > >> > >> Lets say just_expand is true and can_merge_left is true. Now we will adjust > >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't > >> restore vmg->vma/start/pgoff, since just_expand is true. > >> > >> Is this what you expect? > > > >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't > >realise :) > > > >Actually at this point, I don't think we need a follow up patch, sorry. > > > >As I think perhaps I will make this change as part of an existing series > >where I am reworking mmap_region(), since this is the only place where it > >matters, and it would make everything a hell of a lot clearer. > > > >Thanks for pointing this out, it's very useful (and an embarrassing > >oversight on my part...!), but I think it'd be better reworked this way. > > > >Thanks! > > Ok, for now I would just remove these two lines with the change log you > suggested. NO! Sorry I've not been clear - don't send any series. I am going to make a change that eliminates the need for your change (sorry, but in discussing this I've realised that's the best way forward). > > -- > Wei Yang > Help you, Help me >
On Fri, Oct 25, 2024 at 09:51:12AM +0100, Lorenzo Stoakes wrote: >On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote: >> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote: >> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote: >> > >> >> >> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. >> >> >> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust >> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't >> >> restore vmg->vma/start/pgoff, since just_expand is true. >> >> >> >> Is this what you expect? >> > >> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't >> >realise :) >> > >> >Actually at this point, I don't think we need a follow up patch, sorry. >> > >> >As I think perhaps I will make this change as part of an existing series >> >where I am reworking mmap_region(), since this is the only place where it >> >matters, and it would make everything a hell of a lot clearer. >> > >> >Thanks for pointing this out, it's very useful (and an embarrassing >> >oversight on my part...!), but I think it'd be better reworked this way. >> > >> >Thanks! >> >> Ok, for now I would just remove these two lines with the change log you >> suggested. > >NO! Sorry I've not been clear - don't send any series. > >I am going to make a change that eliminates the need for your change (sorry, but >in discussing this I've realised that's the best way forward). > Sure, go ahead. >> >> -- >> Wei Yang >> Help you, Help me >>
On Fri, Oct 25, 2024 at 08:54:09AM +0000, Wei Yang wrote: > On Fri, Oct 25, 2024 at 09:51:12AM +0100, Lorenzo Stoakes wrote: > >On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote: > >> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote: > >> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote: > >> > > >> >> > >> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. > >> >> > >> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust > >> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't > >> >> restore vmg->vma/start/pgoff, since just_expand is true. > >> >> > >> >> Is this what you expect? > >> > > >> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't > >> >realise :) > >> > > >> >Actually at this point, I don't think we need a follow up patch, sorry. > >> > > >> >As I think perhaps I will make this change as part of an existing series > >> >where I am reworking mmap_region(), since this is the only place where it > >> >matters, and it would make everything a hell of a lot clearer. > >> > > >> >Thanks for pointing this out, it's very useful (and an embarrassing > >> >oversight on my part...!), but I think it'd be better reworked this way. > >> > > >> >Thanks! > >> > >> Ok, for now I would just remove these two lines with the change log you > >> suggested. > > > >NO! Sorry I've not been clear - don't send any series. > > > >I am going to make a change that eliminates the need for your change (sorry, but > >in discussing this I've realised that's the best way forward). > > > > Sure, go ahead. Thanks, apologies I know it sucks, to be clear I very much appreciate your input here! For more detail - we are refactoring how the '2nd attempt' at a merge works, also avoiding any abuse of vmg-> fields to keep track of start/end of a range, so it becomes better to just open-code 'reset' of these fields precisely at the point we need them. Do please let us know if you notice any other silly mistakes like this :>) Thanks, Lorenzo > > >> > >> -- > >> Wei Yang > >> Help you, Help me > >> > > -- > Wei Yang > Help you, Help me >
On Fri, Oct 25, 2024 at 10:01:36AM +0100, Lorenzo Stoakes wrote: >On Fri, Oct 25, 2024 at 08:54:09AM +0000, Wei Yang wrote: >> On Fri, Oct 25, 2024 at 09:51:12AM +0100, Lorenzo Stoakes wrote: >> >On Fri, Oct 25, 2024 at 08:49:19AM +0000, Wei Yang wrote: >> >> On Fri, Oct 25, 2024 at 09:40:25AM +0100, Lorenzo Stoakes wrote: >> >> >On Fri, Oct 25, 2024 at 08:32:27AM +0000, Wei Yang wrote: >> >> > >> >> >> >> >> >> But I have a question on your introduction of VMG_FLAG_JUST_EXPAND. >> >> >> >> >> >> Lets say just_expand is true and can_merge_left is true. Now we will adjust >> >> >> vmg->start/vma/pgoff in if (can_merge_left). If we fail expansion, we won't >> >> >> restore vmg->vma/start/pgoff, since just_expand is true. >> >> >> >> >> >> Is this what you expect? >> >> > >> >> >Yes, I explicitly wrote it to do that so it'd be a bit odd if I didn't >> >> >realise :) >> >> > >> >> >Actually at this point, I don't think we need a follow up patch, sorry. >> >> > >> >> >As I think perhaps I will make this change as part of an existing series >> >> >where I am reworking mmap_region(), since this is the only place where it >> >> >matters, and it would make everything a hell of a lot clearer. >> >> > >> >> >Thanks for pointing this out, it's very useful (and an embarrassing >> >> >oversight on my part...!), but I think it'd be better reworked this way. >> >> > >> >> >Thanks! >> >> >> >> Ok, for now I would just remove these two lines with the change log you >> >> suggested. >> > >> >NO! Sorry I've not been clear - don't send any series. >> > >> >I am going to make a change that eliminates the need for your change (sorry, but >> >in discussing this I've realised that's the best way forward). >> > >> >> Sure, go ahead. > >Thanks, apologies I know it sucks, to be clear I very much appreciate your >input here! > Ha, np. Glad to talk with you. >For more detail - we are refactoring how the '2nd attempt' at a merge >works, also avoiding any abuse of vmg-> fields to keep track of start/end >of a range, so it becomes better to just open-code 'reset' of these fields >precisely at the point we need them. > I am not sure we mention the same thing. One confusion during code reading is on the vmg->start/end and vmg->vmi->mas->index/last. For many times I lost who is who... >Do please let us know if you notice any other silly mistakes like this :>) > Let me clear my glasses :-) >Thanks, Lorenzo > >> >> >> >> >> -- >> >> Wei Yang >> >> Help you, Help me >> >> >> >> -- >> Wei Yang >> Help you, Help me >>
diff --git a/mm/vma.c b/mm/vma.c index fb4f1863f88e..c94d953d453c 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -954,23 +954,27 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) vma_prev(vmg->vmi); /* Equivalent to going to the previous range */ } + /* No mergeable adjacent VMA, return */ + if (!vmg->vma) + return NULL; + /* * Now try to expand adjacent VMA(s). This takes care of removing the * following VMA if we have VMAs on both sides. */ - if (vmg->vma && !vma_expand(vmg)) { + if (!vma_expand(vmg)) { khugepaged_enter_vma(vmg->vma, vmg->flags); vmg->state = VMA_MERGE_SUCCESS; return vmg->vma; } /* If expansion failed, reset state. Allows us to retry merge later. */ + if (vmg->vma == prev) + vma_iter_set(vmg->vmi, start); vmg->vma = NULL; vmg->start = start; vmg->end = end; vmg->pgoff = pgoff; - if (vmg->vma == prev) - vma_iter_set(vmg->vmi, start); return NULL; }
On expansion failure, we try to restore vmg state, but we missed to restore vmi.index. The reason is we have reset vmg->vma before checking. So let's put the operation before reset vmg->vma. Also we don't need to do the restore if there is no mergeable adjacent VMA. Let's take it out to skip the unnecessary operations. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- mm/vma.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)