| Message ID | 20180816100628.26428-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memory_hotplug: online/offline_pages refactorings | expand |
Hi David, On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <david@redhat.com> wrote: > > -static int __ref __offline_pages(unsigned long start_pfn, > - unsigned long end_pfn) > +/* Must be protected by mem_hotplug_begin() or a device_lock */ > +int offline_pages(unsigned long start_pfn, unsigned long nr_pages) You lose the __ref marking. Does this introduce warnings since offline_pages() calls (at least) zone_pcp_update() which is marked __meminit.
On 16.08.2018 13:44, Stephen Rothwell wrote: > Hi David, > > On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <david@redhat.com> wrote: >> >> -static int __ref __offline_pages(unsigned long start_pfn, >> - unsigned long end_pfn) >> +/* Must be protected by mem_hotplug_begin() or a device_lock */ >> +int offline_pages(unsigned long start_pfn, unsigned long nr_pages) > > You lose the __ref marking. Does this introduce warnings since > offline_pages() calls (at least) zone_pcp_update() which is marked > __meminit. > Good point, I'll recompile and in case there is a warning, keep the __ref. Thanks!
Hi David,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.18 next-20180816]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Hildenbrand/mm-memory_hotplug-online-offline_pages-refactorings/20180817-002307
config: i386-randconfig-a0-201832 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
>> WARNING: vmlinux.o(.text+0x175eea): Section mismatch in reference from the function offline_pages() to the function .meminit.text:init_per_zone_wmark_min()
The function offline_pages() references
the function __meminit init_per_zone_wmark_min().
This is often because offline_pages lacks a __meminit
annotation or the annotation of init_per_zone_wmark_min is wrong.
--
>> WARNING: vmlinux.o(.text+0x17624f): Section mismatch in reference from the function offline_pages() to the function .meminit.text:zone_pcp_update()
The function offline_pages() references
the function __meminit zone_pcp_update().
This is often because offline_pages lacks a __meminit
annotation or the annotation of zone_pcp_update is wrong.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
I guess the wrap was done because of __ref, but no reason to have this wrap. So looks good to me. Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com> On 8/16/18 6:06 AM, David Hildenbrand wrote: > Let's avoid this indirection and just call the function offline_pages(). > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/memory_hotplug.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 6a2726920ed2..090cf474de87 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg) > node_clear_state(node, N_MEMORY); > } > > -static int __ref __offline_pages(unsigned long start_pfn, > - unsigned long end_pfn) > +/* Must be protected by mem_hotplug_begin() or a device_lock */ > +int offline_pages(unsigned long start_pfn, unsigned long nr_pages) > { > - unsigned long pfn, nr_pages; > + unsigned long pfn, end_pfn = start_pfn + nr_pages; > long offlined_pages; > int ret, node; > unsigned long flags; > @@ -1612,7 +1612,6 @@ static int __ref __offline_pages(unsigned long start_pfn, > > zone = page_zone(pfn_to_page(valid_start)); > node = zone_to_nid(zone); > - nr_pages = end_pfn - start_pfn; > > /* set above range as isolated */ > ret = start_isolate_page_range(start_pfn, end_pfn, > @@ -1700,12 +1699,6 @@ static int __ref __offline_pages(unsigned long start_pfn, > undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE); > return ret; > } > - > -/* Must be protected by mem_hotplug_begin() or a device_lock */ > -int offline_pages(unsigned long start_pfn, unsigned long nr_pages) > -{ > - return __offline_pages(start_pfn, start_pfn + nr_pages); > -} > #endif /* CONFIG_MEMORY_HOTREMOVE */ > > /** >
On 8/30/18 4:17 PM, Pasha Tatashin wrote: > I guess the wrap was done because of __ref, but no reason to have this > wrap. So looks good to me. > > Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>> > On 8/16/18 6:06 AM, David Hildenbrand wrote: >> Let's avoid this indirection and just call the function offline_pages(). >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> mm/memory_hotplug.c | 13 +++---------- >> 1 file changed, 3 insertions(+), 10 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index 6a2726920ed2..090cf474de87 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg) >> node_clear_state(node, N_MEMORY); >> } >> >> -static int __ref __offline_pages(unsigned long start_pfn, >> - unsigned long end_pfn) >> +/* Must be protected by mem_hotplug_begin() or a device_lock */ >> +int offline_pages(unsigned long start_pfn, unsigned long nr_pages) ^^^ I meant to say keep the __ref, otherwise looks good. Thank you, Pavel
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 6a2726920ed2..090cf474de87 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg) node_clear_state(node, N_MEMORY); } -static int __ref __offline_pages(unsigned long start_pfn, - unsigned long end_pfn) +/* Must be protected by mem_hotplug_begin() or a device_lock */ +int offline_pages(unsigned long start_pfn, unsigned long nr_pages) { - unsigned long pfn, nr_pages; + unsigned long pfn, end_pfn = start_pfn + nr_pages; long offlined_pages; int ret, node; unsigned long flags; @@ -1612,7 +1612,6 @@ static int __ref __offline_pages(unsigned long start_pfn, zone = page_zone(pfn_to_page(valid_start)); node = zone_to_nid(zone); - nr_pages = end_pfn - start_pfn; /* set above range as isolated */ ret = start_isolate_page_range(start_pfn, end_pfn, @@ -1700,12 +1699,6 @@ static int __ref __offline_pages(unsigned long start_pfn, undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE); return ret; } - -/* Must be protected by mem_hotplug_begin() or a device_lock */ -int offline_pages(unsigned long start_pfn, unsigned long nr_pages) -{ - return __offline_pages(start_pfn, start_pfn + nr_pages); -} #endif /* CONFIG_MEMORY_HOTREMOVE */ /**
Let's avoid this indirection and just call the function offline_pages(). Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)