| Message ID | 20180816100628.26428-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memory_hotplug: online/offline_pages refactorings | expand |
On Thu, Aug 16, 2018 at 12:06:25PM +0200, David Hildenbrand wrote: > onlining/offlining code works on whole sections, so let's enforce that. > Existing code only allows to add memory in memory block size. And only > whole memory blocks can be onlined/offlined. Memory blocks are always > aligned to sections, so this should not break anything. > > online_pages/offline_pages will implicitly mark whole sections > online/offline, so the code really can only handle such granularities. > > (especially offlining code cannot deal with pageblock_nr_pages but > theoretically only MAX_ORDER-1) > > Signed-off-by: David Hildenbrand <david@redhat.com> Hi David, If you are really willing to move the checks from this patch[1] to online/offline_pages, you might consider to put that in as well. So we have a function that checks for everything, and not multiple checks. Another thing is that I would have prefered to take the checks up to memory_block_action, but offline_pages gets also called from ppc-memtrace code. Other than that, Reviewed-by: Oscar Salvador <osalvador@suse.de> [1] https://patchwork.kernel.org/patch/10567277/ Thanks
Hi David, I am not sure this is needed, because we already have a stricter checker: check_hotplug_memory_range() You could call it from online_pages(), if you think there is a reason to do it, but other than that it is done from add_memory_resource() and from remove_memory(). Thank you, Pavel On 8/16/18 6:06 AM, David Hildenbrand wrote: > onlining/offlining code works on whole sections, so let's enforce that. > Existing code only allows to add memory in memory block size. And only > whole memory blocks can be onlined/offlined. Memory blocks are always > aligned to sections, so this should not break anything. > > online_pages/offline_pages will implicitly mark whole sections > online/offline, so the code really can only handle such granularities. > > (especially offlining code cannot deal with pageblock_nr_pages but > theoretically only MAX_ORDER-1) > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/memory_hotplug.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 090cf474de87..30d2fa42b0bb 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ > struct memory_notify arg; > struct memory_block *mem; > > + if (!IS_ALIGNED(pfn, PAGES_PER_SECTION)) > + return -EINVAL; > + if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION)) > + return -EINVAL; > + > /* > * We can't use pfn_to_nid() because nid might be stored in struct page > * which is not yet initialized. Instead, we find nid from memory block. > @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) > struct zone *zone; > struct memory_notify arg; > > - /* at least, alignment against pageblock is necessary */ > - if (!IS_ALIGNED(start_pfn, pageblock_nr_pages)) > + if (!IS_ALIGNED(start_pfn, PAGES_PER_SECTION)) > return -EINVAL; > - if (!IS_ALIGNED(end_pfn, pageblock_nr_pages)) > + if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION)) > return -EINVAL; > /* This makes hotplug much easier...and readable. > we assume this for now. .*/ >
On 31.08.2018 00:14, Pasha Tatashin wrote: > Hi David, > > I am not sure this is needed, because we already have a stricter checker: > > check_hotplug_memory_range() > > You could call it from online_pages(), if you think there is a reason to > do it, but other than that it is done from add_memory_resource() and > from remove_memory(). Hi, As offline_pages() is called from a different location for ppc (and I understand why but don't consider this clean) and I used both functions in a prototype, believing they would work with pageblock_nr_pages, I really think that we should at least drop the misleading check from offline_pages() and better also add checks for check_hotplug_memory_range(). Thanks for having a look Pavel! > > Thank you, > Pavel > > On 8/16/18 6:06 AM, David Hildenbrand wrote: >> onlining/offlining code works on whole sections, so let's enforce that. >> Existing code only allows to add memory in memory block size. And only >> whole memory blocks can be onlined/offlined. Memory blocks are always >> aligned to sections, so this should not break anything. >> >> online_pages/offline_pages will implicitly mark whole sections >> online/offline, so the code really can only handle such granularities. >> >> (especially offlining code cannot deal with pageblock_nr_pages but >> theoretically only MAX_ORDER-1) >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> mm/memory_hotplug.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index 090cf474de87..30d2fa42b0bb 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ >> struct memory_notify arg; >> struct memory_block *mem; >> >> + if (!IS_ALIGNED(pfn, PAGES_PER_SECTION)) >> + return -EINVAL; >> + if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION)) >> + return -EINVAL; >> + >> /* >> * We can't use pfn_to_nid() because nid might be stored in struct page >> * which is not yet initialized. Instead, we find nid from memory block. >> @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) >> struct zone *zone; >> struct memory_notify arg; >> >> - /* at least, alignment against pageblock is necessary */ >> - if (!IS_ALIGNED(start_pfn, pageblock_nr_pages)) >> + if (!IS_ALIGNED(start_pfn, PAGES_PER_SECTION)) >> return -EINVAL; >> - if (!IS_ALIGNED(end_pfn, pageblock_nr_pages)) >> + if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION)) >> return -EINVAL; >> /* This makes hotplug much easier...and readable. >> we assume this for now. .*/
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 090cf474de87..30d2fa42b0bb 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ struct memory_notify arg; struct memory_block *mem; + if (!IS_ALIGNED(pfn, PAGES_PER_SECTION)) + return -EINVAL; + if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION)) + return -EINVAL; + /* * We can't use pfn_to_nid() because nid might be stored in struct page * which is not yet initialized. Instead, we find nid from memory block. @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) struct zone *zone; struct memory_notify arg; - /* at least, alignment against pageblock is necessary */ - if (!IS_ALIGNED(start_pfn, pageblock_nr_pages)) + if (!IS_ALIGNED(start_pfn, PAGES_PER_SECTION)) return -EINVAL; - if (!IS_ALIGNED(end_pfn, pageblock_nr_pages)) + if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION)) return -EINVAL; /* This makes hotplug much easier...and readable. we assume this for now. .*/
onlining/offlining code works on whole sections, so let's enforce that. Existing code only allows to add memory in memory block size. And only whole memory blocks can be onlined/offlined. Memory blocks are always aligned to sections, so this should not break anything. online_pages/offline_pages will implicitly mark whole sections online/offline, so the code really can only handle such granularities. (especially offlining code cannot deal with pageblock_nr_pages but theoretically only MAX_ORDER-1) Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)