| Message ID | 20210416112411.9826-3-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9,1/8] drivers/base/memory: Introduce memory_block_{online,offline} | expand |
On Fri 16-04-21 13:24:05, Oscar Salvador wrote: > When using self-hosted vmemmap pages, the number of pages passed to > {online,offline}_pages might not fully span sections, but they always > fully span pageblocks. > Relax the check account for that case. It would be good to call those out explicitly. It would be also great to explain why pageblock_nr_pages is an actual constrain. There shouldn't be any real reason for that except for "we want online_pages to operate on whole memblocks and memmap_on_memory will poke pageblock_nr_pages aligned holes in the beginning which is a special case we want to allow." > Signed-off-by: Oscar Salvador <osalvador@suse.de> > Reviewed-by: David Hildenbrand <david@redhat.com> With the changelog extended and the comment clarification (se below) feel free to add Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/memory_hotplug.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 0cdbbfbc5757..25e59d5dc13c 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -838,9 +838,14 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, > int ret; > struct memory_notify arg; > > - /* We can only online full sections (e.g., SECTION_IS_ONLINE) */ > + /* We can only offline full sections (e.g., SECTION_IS_ONLINE). > + * However, when using e.g: memmap_on_memory, some pages are initialized > + * prior to calling in here. The remaining amount of pages must be > + * pageblock aligned. I would rephrase (and also note that multi line comment usually have a leading line without any content - not that I care much though). /* * {on,off}lining is constrained to full memory sections (or * more precisly to memory blocks from the user space POV). * memmap_on_memory is an exception because it reserves initial * part of the physical memory space for vmemmaps. That space is * pageblock aligned. > + */ Same comment would apply to oofline_pages. > if (WARN_ON_ONCE(!nr_pages || > - !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION))) > + !IS_ALIGNED(pfn, pageblock_nr_pages) || > + !IS_ALIGNED(pfn + nr_pages, PAGES_PER_SECTION))) > return -EINVAL; > > mem_hotplug_begin(); > @@ -1573,9 +1578,14 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages) > int ret, node; > char *reason; > > - /* We can only offline full sections (e.g., SECTION_IS_ONLINE) */ > + /* We can only offline full sections (e.g., SECTION_IS_ONLINE). > + * However, when using e.g: memmap_on_memory, some pages are initialized > + * prior to calling in here. The remaining amount of pages must be > + * pageblock aligned. > + */ > if (WARN_ON_ONCE(!nr_pages || > - !IS_ALIGNED(start_pfn | nr_pages, PAGES_PER_SECTION))) > + !IS_ALIGNED(start_pfn, pageblock_nr_pages) || > + !IS_ALIGNED(start_pfn + nr_pages, PAGES_PER_SECTION))) > return -EINVAL; > > mem_hotplug_begin(); > -- > 2.16.3
On Tue, Apr 20, 2021 at 11:40:50AM +0200, Michal Hocko wrote: > On Fri 16-04-21 13:24:05, Oscar Salvador wrote: > > When using self-hosted vmemmap pages, the number of pages passed to > > {online,offline}_pages might not fully span sections, but they always > > fully span pageblocks. > > Relax the check account for that case. > > It would be good to call those out explicitly. It would be also > great to explain why pageblock_nr_pages is an actual constrain. There > shouldn't be any real reason for that except for "we want online_pages > to operate on whole memblocks and memmap_on_memory will poke > pageblock_nr_pages aligned holes in the beginning which is a special > case we want to allow." Sounds good. > > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > > Reviewed-by: David Hildenbrand <david@redhat.com> > > With the changelog extended and the comment clarification (se below) > feel free to add Ok, thanks for the suggestion Michal. > Acked-by: Michal Hocko <mhocko@suse.com>
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 0cdbbfbc5757..25e59d5dc13c 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -838,9 +838,14 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int ret; struct memory_notify arg; - /* We can only online full sections (e.g., SECTION_IS_ONLINE) */ + /* We can only offline full sections (e.g., SECTION_IS_ONLINE). + * However, when using e.g: memmap_on_memory, some pages are initialized + * prior to calling in here. The remaining amount of pages must be + * pageblock aligned. + */ if (WARN_ON_ONCE(!nr_pages || - !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION))) + !IS_ALIGNED(pfn, pageblock_nr_pages) || + !IS_ALIGNED(pfn + nr_pages, PAGES_PER_SECTION))) return -EINVAL; mem_hotplug_begin(); @@ -1573,9 +1578,14 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages) int ret, node; char *reason; - /* We can only offline full sections (e.g., SECTION_IS_ONLINE) */ + /* We can only offline full sections (e.g., SECTION_IS_ONLINE). + * However, when using e.g: memmap_on_memory, some pages are initialized + * prior to calling in here. The remaining amount of pages must be + * pageblock aligned. + */ if (WARN_ON_ONCE(!nr_pages || - !IS_ALIGNED(start_pfn | nr_pages, PAGES_PER_SECTION))) + !IS_ALIGNED(start_pfn, pageblock_nr_pages) || + !IS_ALIGNED(start_pfn + nr_pages, PAGES_PER_SECTION))) return -EINVAL; mem_hotplug_begin();
When using self-hosted vmemmap pages, the number of pages passed to {online,offline}_pages might not fully span sections, but they always fully span pageblocks. Relax the check account for that case. Signed-off-by: Oscar Salvador <osalvador@suse.de> Reviewed-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)