* [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
@ 2021-09-13 11:51 Miaohe Lin
2021-09-13 12:12 ` Michal Hocko
0 siblings, 1 reply; 10+ messages in thread
From: Miaohe Lin @ 2021-09-13 11:51 UTC (permalink / raw)
To: akpm; +Cc: david, mhocko, vbabka, linux-mm, linux-kernel, linmiaohe
In start_isolate_page_range() undo path, pfn_to_online_page() just checks
the first pfn in a pageblock while __first_valid_page() will traverse the
pageblock until the first online pfn is found. So we may miss the call to
unset_migratetype_isolate() in undo path and pages will remain isolated
unexpectedly. Fix this by calling undo_isolate_page_range() and this will
also help to simplify the code further.
Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Cc: <stable@vger.kernel.org>
---
v1->v2:
Simplify the code further per David Hildenbrand.
---
mm/page_isolation.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index a95c2c6562d0..f93cc63d8fa1 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -183,7 +183,6 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
unsigned migratetype, int flags)
{
unsigned long pfn;
- unsigned long undo_pfn;
struct page *page;
BUG_ON(!IS_ALIGNED(start_pfn, pageblock_nr_pages));
@@ -193,25 +192,12 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
pfn < end_pfn;
pfn += pageblock_nr_pages) {
page = __first_valid_page(pfn, pageblock_nr_pages);
- if (page) {
- if (set_migratetype_isolate(page, migratetype, flags)) {
- undo_pfn = pfn;
- goto undo;
- }
+ if (page && set_migratetype_isolate(page, migratetype, flags)) {
+ undo_isolate_page_range(start_pfn, pfn, migratetype);
+ return -EBUSY;
}
}
return 0;
-undo:
- for (pfn = start_pfn;
- pfn < undo_pfn;
- pfn += pageblock_nr_pages) {
- struct page *page = pfn_to_online_page(pfn);
- if (!page)
- continue;
- unset_migratetype_isolate(page, migratetype);
- }
-
- return -EBUSY;
}
/*
--
2.23.0
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-13 11:51 [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate() Miaohe Lin
@ 2021-09-13 12:12 ` Michal Hocko
2021-09-13 12:20 ` David Hildenbrand
0 siblings, 1 reply; 10+ messages in thread
From: Michal Hocko @ 2021-09-13 12:12 UTC (permalink / raw)
To: Miaohe Lin; +Cc: akpm, david, vbabka, linux-mm, linux-kernel
On Mon 13-09-21 19:51:25, Miaohe Lin wrote:
> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
> the first pfn in a pageblock while __first_valid_page() will traverse the
> pageblock until the first online pfn is found. So we may miss the call to
> unset_migratetype_isolate() in undo path and pages will remain isolated
> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
> also help to simplify the code further.
I like the clean up part but is this a real problem that requires CC
stable? Have you ever seen this to be a real problem? It looks like
something based on reading the code.
> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> Cc: <stable@vger.kernel.org>
> ---
> v1->v2:
> Simplify the code further per David Hildenbrand.
> ---
> mm/page_isolation.c | 20 +++-----------------
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index a95c2c6562d0..f93cc63d8fa1 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -183,7 +183,6 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
> unsigned migratetype, int flags)
> {
> unsigned long pfn;
> - unsigned long undo_pfn;
> struct page *page;
>
> BUG_ON(!IS_ALIGNED(start_pfn, pageblock_nr_pages));
> @@ -193,25 +192,12 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
> pfn < end_pfn;
> pfn += pageblock_nr_pages) {
> page = __first_valid_page(pfn, pageblock_nr_pages);
> - if (page) {
> - if (set_migratetype_isolate(page, migratetype, flags)) {
> - undo_pfn = pfn;
> - goto undo;
> - }
> + if (page && set_migratetype_isolate(page, migratetype, flags)) {
> + undo_isolate_page_range(start_pfn, pfn, migratetype);
> + return -EBUSY;
> }
> }
> return 0;
> -undo:
> - for (pfn = start_pfn;
> - pfn < undo_pfn;
> - pfn += pageblock_nr_pages) {
> - struct page *page = pfn_to_online_page(pfn);
> - if (!page)
> - continue;
> - unset_migratetype_isolate(page, migratetype);
> - }
> -
> - return -EBUSY;
> }
>
> /*
> --
> 2.23.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-13 12:12 ` Michal Hocko
@ 2021-09-13 12:20 ` David Hildenbrand
2021-09-13 12:43 ` Miaohe Lin
0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2021-09-13 12:20 UTC (permalink / raw)
To: Michal Hocko, Miaohe Lin; +Cc: akpm, vbabka, linux-mm, linux-kernel
On 13.09.21 14:12, Michal Hocko wrote:
> On Mon 13-09-21 19:51:25, Miaohe Lin wrote:
>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
>> the first pfn in a pageblock while __first_valid_page() will traverse the
>> pageblock until the first online pfn is found. So we may miss the call to
>> unset_migratetype_isolate() in undo path and pages will remain isolated
>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
>> also help to simplify the code further.
>
> I like the clean up part but is this a real problem that requires CC
> stable? Have you ever seen this to be a real problem? It looks like
> something based on reading the code.
We discussed that it isn't an issue anymore (we never call it on memory
holes), but might have been an issue on older kernels, back when we
didn't have the "memory holes" check in the memory offlining path in place.
Agreed, these details belong into this description.
>
>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> v1->v2:
>> Simplify the code further per David Hildenbrand.
>> ---
>> mm/page_isolation.c | 20 +++-----------------
>> 1 file changed, 3 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>> index a95c2c6562d0..f93cc63d8fa1 100644
>> --- a/mm/page_isolation.c
>> +++ b/mm/page_isolation.c
>> @@ -183,7 +183,6 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>> unsigned migratetype, int flags)
>> {
>> unsigned long pfn;
>> - unsigned long undo_pfn;
>> struct page *page;
>>
>> BUG_ON(!IS_ALIGNED(start_pfn, pageblock_nr_pages));
>> @@ -193,25 +192,12 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>> pfn < end_pfn;
>> pfn += pageblock_nr_pages) {
>> page = __first_valid_page(pfn, pageblock_nr_pages);
>> - if (page) {
>> - if (set_migratetype_isolate(page, migratetype, flags)) {
>> - undo_pfn = pfn;
>> - goto undo;
>> - }
>> + if (page && set_migratetype_isolate(page, migratetype, flags)) {
>> + undo_isolate_page_range(start_pfn, pfn, migratetype);
>> + return -EBUSY;
>> }
>> }
>> return 0;
>> -undo:
>> - for (pfn = start_pfn;
>> - pfn < undo_pfn;
>> - pfn += pageblock_nr_pages) {
>> - struct page *page = pfn_to_online_page(pfn);
>> - if (!page)
>> - continue;
>> - unset_migratetype_isolate(page, migratetype);
>> - }
>> -
>> - return -EBUSY;
>> }
>>
>> /*
>> --
>> 2.23.0
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-13 12:20 ` David Hildenbrand
@ 2021-09-13 12:43 ` Miaohe Lin
2021-09-13 12:59 ` Michal Hocko
0 siblings, 1 reply; 10+ messages in thread
From: Miaohe Lin @ 2021-09-13 12:43 UTC (permalink / raw)
To: David Hildenbrand, Michal Hocko; +Cc: akpm, vbabka, linux-mm, linux-kernel
On 2021/9/13 20:20, David Hildenbrand wrote:
> On 13.09.21 14:12, Michal Hocko wrote:
>> On Mon 13-09-21 19:51:25, Miaohe Lin wrote:
>>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
>>> the first pfn in a pageblock while __first_valid_page() will traverse the
>>> pageblock until the first online pfn is found. So we may miss the call to
>>> unset_migratetype_isolate() in undo path and pages will remain isolated
>>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
>>> also help to simplify the code further.
>>
>> I like the clean up part but is this a real problem that requires CC
>> stable? Have you ever seen this to be a real problem? It looks like
>> something based on reading the code.
I'm sorry but I haven't seen this to be a real problem. It's a theoretical bug.
>
> We discussed that it isn't an issue anymore (we never call it on memory holes), but might have been an issue on older kernels, back when we didn't have the "memory holes" check in the memory offlining path in place.
So is the Cc:stable needed in this case?
Many thanks for both of you.
>
> Agreed, these details belong into this description.
>
>>
>>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>> v1->v2:
>>> Simplify the code further per David Hildenbrand.
>>> ---
>>> mm/page_isolation.c | 20 +++-----------------
>>> 1 file changed, 3 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index a95c2c6562d0..f93cc63d8fa1 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -183,7 +183,6 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>>> unsigned migratetype, int flags)
>>> {
>>> unsigned long pfn;
>>> - unsigned long undo_pfn;
>>> struct page *page;
>>> BUG_ON(!IS_ALIGNED(start_pfn, pageblock_nr_pages));
>>> @@ -193,25 +192,12 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>>> pfn < end_pfn;
>>> pfn += pageblock_nr_pages) {
>>> page = __first_valid_page(pfn, pageblock_nr_pages);
>>> - if (page) {
>>> - if (set_migratetype_isolate(page, migratetype, flags)) {
>>> - undo_pfn = pfn;
>>> - goto undo;
>>> - }
>>> + if (page && set_migratetype_isolate(page, migratetype, flags)) {
>>> + undo_isolate_page_range(start_pfn, pfn, migratetype);
>>> + return -EBUSY;
>>> }
>>> }
>>> return 0;
>>> -undo:
>>> - for (pfn = start_pfn;
>>> - pfn < undo_pfn;
>>> - pfn += pageblock_nr_pages) {
>>> - struct page *page = pfn_to_online_page(pfn);
>>> - if (!page)
>>> - continue;
>>> - unset_migratetype_isolate(page, migratetype);
>>> - }
>>> -
>>> - return -EBUSY;
>>> }
>>> /*
>>> --
>>> 2.23.0
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-13 12:43 ` Miaohe Lin
@ 2021-09-13 12:59 ` Michal Hocko
2021-09-14 2:51 ` Andrew Morton
2021-09-14 3:09 ` Miaohe Lin
0 siblings, 2 replies; 10+ messages in thread
From: Michal Hocko @ 2021-09-13 12:59 UTC (permalink / raw)
To: Miaohe Lin; +Cc: David Hildenbrand, akpm, vbabka, linux-mm, linux-kernel
On Mon 13-09-21 20:43:35, Miaohe Lin wrote:
> On 2021/9/13 20:20, David Hildenbrand wrote:
> > On 13.09.21 14:12, Michal Hocko wrote:
> >> On Mon 13-09-21 19:51:25, Miaohe Lin wrote:
> >>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
> >>> the first pfn in a pageblock while __first_valid_page() will traverse the
> >>> pageblock until the first online pfn is found. So we may miss the call to
> >>> unset_migratetype_isolate() in undo path and pages will remain isolated
> >>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
> >>> also help to simplify the code further.
> >>
> >> I like the clean up part but is this a real problem that requires CC
> >> stable? Have you ever seen this to be a real problem? It looks like
> >> something based on reading the code.
>
> I'm sorry but I haven't seen this to be a real problem. It's a theoretical bug.
Make it clear in the changelog
> > We discussed that it isn't an issue anymore (we never call it on
> > memory holes), but might have been an issue on older kernels, back
> > when we didn't have the "memory holes" check in the memory offlining
> > path in place.
>
> So is the Cc:stable needed in this case?
I do not think so. Even if this was happening in the practice then the
practical consequences would be pretty minor, right? (few pageblocks
stay isolated thus unavailable).
I do realize that the stable tree is in a hoarding mode for quite some
years but my general approach has been (in line with the documentation)
to mark and backport only fixes that really do matter.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-13 12:59 ` Michal Hocko
@ 2021-09-14 2:51 ` Andrew Morton
2021-09-14 3:10 ` Miaohe Lin
2021-09-14 3:09 ` Miaohe Lin
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2021-09-14 2:51 UTC (permalink / raw)
To: Michal Hocko
Cc: Miaohe Lin, David Hildenbrand, vbabka, linux-mm, linux-kernel
On Mon, 13 Sep 2021 14:59:42 +0200 Michal Hocko <mhocko@suse.com> wrote:
> I do realize that the stable tree is in a hoarding mode for quite some
> years but my general approach has been (in line with the documentation)
> to mark and backport only fixes that really do matter.
Me2. There has to be some risk-vs-reward test to be passed...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-14 2:51 ` Andrew Morton
@ 2021-09-14 3:10 ` Miaohe Lin
0 siblings, 0 replies; 10+ messages in thread
From: Miaohe Lin @ 2021-09-14 3:10 UTC (permalink / raw)
To: Andrew Morton, Michal Hocko
Cc: David Hildenbrand, vbabka, linux-mm, linux-kernel
On 2021/9/14 10:51, Andrew Morton wrote:
> On Mon, 13 Sep 2021 14:59:42 +0200 Michal Hocko <mhocko@suse.com> wrote:
>
>> I do realize that the stable tree is in a hoarding mode for quite some
>> years but my general approach has been (in line with the documentation)
>> to mark and backport only fixes that really do matter.
>
> Me2. There has to be some risk-vs-reward test to be passed...
Will keep this in mind when I try to cc stable. Thanks.
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-13 12:59 ` Michal Hocko
2021-09-14 2:51 ` Andrew Morton
@ 2021-09-14 3:09 ` Miaohe Lin
2021-09-14 7:06 ` Michal Hocko
1 sibling, 1 reply; 10+ messages in thread
From: Miaohe Lin @ 2021-09-14 3:09 UTC (permalink / raw)
To: Michal Hocko, Andrew Morton
Cc: David Hildenbrand, vbabka, linux-mm, linux-kernel
On 2021/9/13 20:59, Michal Hocko wrote:
> On Mon 13-09-21 20:43:35, Miaohe Lin wrote:
>> On 2021/9/13 20:20, David Hildenbrand wrote:
>>> On 13.09.21 14:12, Michal Hocko wrote:
>>>> On Mon 13-09-21 19:51:25, Miaohe Lin wrote:
>>>>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
>>>>> the first pfn in a pageblock while __first_valid_page() will traverse the
>>>>> pageblock until the first online pfn is found. So we may miss the call to
>>>>> unset_migratetype_isolate() in undo path and pages will remain isolated
>>>>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
>>>>> also help to simplify the code further.
>>>>
>>>> I like the clean up part but is this a real problem that requires CC
>>>> stable? Have you ever seen this to be a real problem? It looks like
>>>> something based on reading the code.
>>
>> I'm sorry but I haven't seen this to be a real problem. It's a theoretical bug.
>
> Make it clear in the changelog
Will do.
>
>>> We discussed that it isn't an issue anymore (we never call it on
>>> memory holes), but might have been an issue on older kernels, back
>>> when we didn't have the "memory holes" check in the memory offlining
>>> path in place.
>>
>> So is the Cc:stable needed in this case?
>
> I do not think so. Even if this was happening in the practice then the
> practical consequences would be pretty minor, right? (few pageblocks
> stay isolated thus unavailable).
>
> I do realize that the stable tree is in a hoarding mode for quite some
> years but my general approach has been (in line with the documentation)
> to mark and backport only fixes that really do matter.
So even the Fixes tag should be removed ?
Many thanks.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-14 3:09 ` Miaohe Lin
@ 2021-09-14 7:06 ` Michal Hocko
2021-09-14 9:27 ` Miaohe Lin
0 siblings, 1 reply; 10+ messages in thread
From: Michal Hocko @ 2021-09-14 7:06 UTC (permalink / raw)
To: Miaohe Lin
Cc: Andrew Morton, David Hildenbrand, vbabka, linux-mm, linux-kernel
On Tue 14-09-21 11:09:47, Miaohe Lin wrote:
[...]
> So even the Fixes tag should be removed ?
I would keep that one there. Fixes tag is useful to frame the scope of
the fix. For example when somebody is backporting the commit mentioned
in the Fixes tag then a) a lot of follow up patches with Fixes can tell
you this won't be an easy ride and you might want to reconsider risks
vs. benefit b) it helps to collect follow up fixes more easily.
That is a different story from cc: stable which just collects patches
and push them to all consumers of the stable branch if they apply.
To conclude, the Fixes tag is a generaly useful tag to bind patches
together and let people evaluate how important that is while Cc stable
is an indication that a fix is serious enough to push to all stable
users.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()
2021-09-14 7:06 ` Michal Hocko
@ 2021-09-14 9:27 ` Miaohe Lin
0 siblings, 0 replies; 10+ messages in thread
From: Miaohe Lin @ 2021-09-14 9:27 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, David Hildenbrand, vbabka, linux-mm, linux-kernel
On 2021/9/14 15:06, Michal Hocko wrote:
> On Tue 14-09-21 11:09:47, Miaohe Lin wrote:
> [...]
>> So even the Fixes tag should be removed ?
>
> I would keep that one there. Fixes tag is useful to frame the scope of
> the fix. For example when somebody is backporting the commit mentioned
> in the Fixes tag then a) a lot of follow up patches with Fixes can tell
> you this won't be an easy ride and you might want to reconsider risks
> vs. benefit b) it helps to collect follow up fixes more easily.
>
> That is a different story from cc: stable which just collects patches
> and push them to all consumers of the stable branch if they apply.
>
> To conclude, the Fixes tag is a generaly useful tag to bind patches
> together and let people evaluate how important that is while Cc stable
> is an indication that a fix is serious enough to push to all stable
> users.
>
I see. Many thanks for your detailed explanation! :)
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-14 9:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13 11:51 [PATCH v2] mm/page_isolation: fix potential missing call to unset_migratetype_isolate() Miaohe Lin
2021-09-13 12:12 ` Michal Hocko
2021-09-13 12:20 ` David Hildenbrand
2021-09-13 12:43 ` Miaohe Lin
2021-09-13 12:59 ` Michal Hocko
2021-09-14 2:51 ` Andrew Morton
2021-09-14 3:10 ` Miaohe Lin
2021-09-14 3:09 ` Miaohe Lin
2021-09-14 7:06 ` Michal Hocko
2021-09-14 9:27 ` Miaohe Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox