Hi all,
This series contains cleanups to remove meaningless VM_BUG_ON(), use
helpers to simplify the code and remove obsolete comment. Also we fix
the potential accessing to uninitialized pcp page migratetype and so
on. More details can be found in the respective changelogs. Thanks!
Miaohe Lin (6):
mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order()
mm/page_alloc.c: simplify the code by using macro K()
mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
mm/page_alloc.c: use helper function zone_spans_pfn()
mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
mm/page_alloc.c: avoid allocating highmem pages via
alloc_pages_exact_nid()
mm/page_alloc.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
--
2.23.0
It's meaningless to VM_BUG_ON() order != pageblock_order just after
setting order to pageblock_order. Remove it.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/page_alloc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 91edb930b8ab..dbb3338d9287 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -677,10 +677,8 @@ static inline int pindex_to_order(unsigned int pindex)
int order = pindex / MIGRATE_PCPTYPES;
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- if (order > PAGE_ALLOC_COSTLY_ORDER) {
+ if (order > PAGE_ALLOC_COSTLY_ORDER)
order = pageblock_order;
- VM_BUG_ON(order != pageblock_order);
- }
#else
VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);
#endif
--
2.23.0
Use helper function zone_spans_pfn() to check whether pfn is within a
zone to simplify the code slightly.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/page_alloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b5edcfe112aa..7bb79e959ab4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1576,7 +1576,7 @@ static void __meminit init_reserved_page(unsigned long pfn)
for (zid = 0; zid < MAX_NR_ZONES; zid++) {
struct zone *zone = &pgdat->node_zones[zid];
- if (pfn >= zone->zone_start_pfn && pfn < zone_end_pfn(zone))
+ if (zone_spans_pfn(zone, pfn))
break;
}
__init_single_page(pfn_to_page(pfn), pfn, zid, nid);
--
2.23.0
Don't use with __GFP_HIGHMEM because page_address() cannot represent
highmem pages without kmap(). Newly allocated pages would leak as
page_address() will return NULL for highmem pages here. But It works
now because the only caller does not specify __GFP_HIGHMEM now.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/page_alloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d87b7e6e9e6b..858fd45ecaea 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5639,7 +5639,7 @@ void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask)
if (WARN_ON_ONCE(gfp_mask & __GFP_COMP))
gfp_mask &= ~__GFP_COMP;
- p = alloc_pages_node(nid, gfp_mask, order);
+ p = alloc_pages_node(nid, gfp_mask & ~__GFP_HIGHMEM, order);
if (!p)
return NULL;
return make_alloc_exact((unsigned long)page_address(p), order, size);
--
2.23.0
On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
> Don't use with __GFP_HIGHMEM because page_address() cannot represent
> highmem pages without kmap(). Newly allocated pages would leak as
> page_address() will return NULL for highmem pages here. But It works
> now because the only caller does not specify __GFP_HIGHMEM now.
This is a misunderstanding of how alloc_pages_exact() /
alloc_pages_exact_nid() work. You simply can't call them with
GFP_HIGHMEM.
If you really must change anything here,
s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
alloc_pages_exact() and alloc_pages_exact_nid().
On 2021/8/30 22:24, Matthew Wilcox wrote:
> On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
>> Don't use with __GFP_HIGHMEM because page_address() cannot represent
>> highmem pages without kmap(). Newly allocated pages would leak as
>> page_address() will return NULL for highmem pages here. But It works
>> now because the only caller does not specify __GFP_HIGHMEM now.
>
> This is a misunderstanding of how alloc_pages_exact() /
> alloc_pages_exact_nid() work. You simply can't call them with
> GFP_HIGHMEM.
>
Yep, they can't work with GFP_HIGHMEM. So IMO it might be better to
get rid of GFP_HIGHMEM explicitly or add a comment to clarify this
situation to avoid future misbehavior. But this may be a unnecessary
worry... Do you prefer to not change anything here?
Many thanks.
> If you really must change anything here,
> s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
> alloc_pages_exact() and alloc_pages_exact_nid().
> .
>
On Mon, Aug 30, 2021 at 10:10:46PM +0800, Miaohe Lin wrote:
> It's meaningless to VM_BUG_ON() order != pageblock_order just after
> setting order to pageblock_order. Remove it.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
On Mon, Aug 30, 2021 at 10:10:49PM +0800, Miaohe Lin wrote:
> Use helper function zone_spans_pfn() to check whether pfn is within a
> zone to simplify the code slightly.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
On 30.08.21 16:10, Miaohe Lin wrote:
> It's meaningless to VM_BUG_ON() order != pageblock_order just after
> setting order to pageblock_order. Remove it.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/page_alloc.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 91edb930b8ab..dbb3338d9287 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -677,10 +677,8 @@ static inline int pindex_to_order(unsigned int pindex)
> int order = pindex / MIGRATE_PCPTYPES;
>
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - if (order > PAGE_ALLOC_COSTLY_ORDER) {
> + if (order > PAGE_ALLOC_COSTLY_ORDER)
> order = pageblock_order;
> - VM_BUG_ON(order != pageblock_order);
> - }
> #else
> VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);
> #endif
>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 30.08.21 16:10, Miaohe Lin wrote:
> Use helper function zone_spans_pfn() to check whether pfn is within a
> zone to simplify the code slightly.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/page_alloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b5edcfe112aa..7bb79e959ab4 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1576,7 +1576,7 @@ static void __meminit init_reserved_page(unsigned long pfn)
> for (zid = 0; zid < MAX_NR_ZONES; zid++) {
> struct zone *zone = &pgdat->node_zones[zid];
>
> - if (pfn >= zone->zone_start_pfn && pfn < zone_end_pfn(zone))
> + if (zone_spans_pfn(zone, pfn))
> break;
> }
> __init_single_page(pfn_to_page(pfn), pfn, zid, nid);
>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 8/31/21 03:56, Miaohe Lin wrote:
> On 2021/8/30 22:24, Matthew Wilcox wrote:
>> On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
>>> Don't use with __GFP_HIGHMEM because page_address() cannot represent
>>> highmem pages without kmap(). Newly allocated pages would leak as
>>> page_address() will return NULL for highmem pages here. But It works
>>> now because the only caller does not specify __GFP_HIGHMEM now.
>>
>> This is a misunderstanding of how alloc_pages_exact() /
>> alloc_pages_exact_nid() work. You simply can't call them with
>> GFP_HIGHMEM.
>>
>
> Yep, they can't work with GFP_HIGHMEM. So IMO it might be better to
> get rid of GFP_HIGHMEM explicitly or add a comment to clarify this
> situation to avoid future misbehavior. But this may be a unnecessary
> worry... Do you prefer to not change anything here?
I agree with the suggestion below...
> Many thanks.
>
>> If you really must change anything here,
>> s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
>> alloc_pages_exact() and alloc_pages_exact_nid().
... which means __GFP_HIGHMEM would be stripped and additionally there would
be a warning.
On 2021/9/1 0:37, Vlastimil Babka wrote:
> On 8/31/21 03:56, Miaohe Lin wrote:
>> On 2021/8/30 22:24, Matthew Wilcox wrote:
>>> On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
>>>> Don't use with __GFP_HIGHMEM because page_address() cannot represent
>>>> highmem pages without kmap(). Newly allocated pages would leak as
>>>> page_address() will return NULL for highmem pages here. But It works
>>>> now because the only caller does not specify __GFP_HIGHMEM now.
>>>
>>> This is a misunderstanding of how alloc_pages_exact() /
>>> alloc_pages_exact_nid() work. You simply can't call them with
>>> GFP_HIGHMEM.
>>>
>>
>> Yep, they can't work with GFP_HIGHMEM. So IMO it might be better to
>> get rid of GFP_HIGHMEM explicitly or add a comment to clarify this
>> situation to avoid future misbehavior. But this may be a unnecessary
>> worry... Do you prefer to not change anything here?
>
> I agree with the suggestion below...
>
>> Many thanks.
>>
>>> If you really must change anything here,
>>> s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
>>> alloc_pages_exact() and alloc_pages_exact_nid().
>
> ... which means __GFP_HIGHMEM would be stripped and additionally there would
> be a warning.
>
Looks good for me. Will do. Many thanks!
> .
>