| Message ID | 20200819101157.12723-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memory_hotplug: online_pages()/offline_pages() cleanups | expand |
On Wed 19-08-20 12:11:48, David Hildenbrand wrote: > Already two people (including me) tried to offline subsections, because > the function looks like it can deal with it. But we really can only > online/offline full sections (e.g., we can only mark full sections > online/offline via SECTION_IS_ONLINE). > > Add a simple safety net that to document the restriction now. Current users > (core and powernv/memtrace) respect these restrictions. I do agree with the warning because it clarifies our expectations indeed. Se below for more questions. > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Wei Yang <richard.weiyang@linux.alibaba.com> > Cc: Baoquan He <bhe@redhat.com> > Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > Cc: Oscar Salvador <osalvador@suse.de> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/memory_hotplug.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index c781d386d87f9..6856702af68d9 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -801,6 +801,11 @@ 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) */ > + if (WARN_ON_ONCE(!nr_pages || > + !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION))) > + return -EINVAL; This looks looks unnecessarily cryptic to me. Do you want to catch full section operation that doesn't start at the usual section boundary? If yes the above doesn't work work unless I am missing something. Why don't you simply WARN_ON_ONCE(nr_pages % PAGES_PER_SECTION). !nr_pages doesn't sound like something interesting to care about or why do we care?
On 19.08.20 14:37, Michal Hocko wrote: > On Wed 19-08-20 12:11:48, David Hildenbrand wrote: >> Already two people (including me) tried to offline subsections, because >> the function looks like it can deal with it. But we really can only >> online/offline full sections (e.g., we can only mark full sections >> online/offline via SECTION_IS_ONLINE). >> >> Add a simple safety net that to document the restriction now. Current users >> (core and powernv/memtrace) respect these restrictions. > > I do agree with the warning because it clarifies our expectations > indeed. Se below for more questions. > >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Wei Yang <richard.weiyang@linux.alibaba.com> >> Cc: Baoquan He <bhe@redhat.com> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >> Cc: Oscar Salvador <osalvador@suse.de> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> mm/memory_hotplug.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index c781d386d87f9..6856702af68d9 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -801,6 +801,11 @@ 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) */ >> + if (WARN_ON_ONCE(!nr_pages || >> + !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION))) >> + return -EINVAL; > > This looks looks unnecessarily cryptic to me. Do you want to catch full > section operation that doesn't start at the usual section boundary? If > yes the above doesn't work work unless I am missing something. > > Why don't you simply WARN_ON_ONCE(nr_pages % PAGES_PER_SECTION). > !nr_pages doesn't sound like something interesting to care about or why > do we care? > Also the start pfn has to be section aligned, so we always cover fully aligned sections (e.g., not two partial ones). It's essentially a compressed version of !nr_pages || !IS_ALIGNED(pfn, PAGES_PER_SECTION) || !IS_ALIGN(nr_pages, PAGES_PER_SECTION) which is the same as !nr_pages || pfn % PAGES_PER_SECTION) || nr_pages % PAGES_PER_SECTION or !nr_pages || (pfn | nr_pages) % PAGES_PER_SECTION I consider IS_ALIGNED easier to read than % PAGES_PER_SECTION. I can certainly un-compress, whatever you prefer, thanks.
On Wed 19-08-20 14:43:28, David Hildenbrand wrote: > On 19.08.20 14:37, Michal Hocko wrote: > > On Wed 19-08-20 12:11:48, David Hildenbrand wrote: > >> Already two people (including me) tried to offline subsections, because > >> the function looks like it can deal with it. But we really can only > >> online/offline full sections (e.g., we can only mark full sections > >> online/offline via SECTION_IS_ONLINE). > >> > >> Add a simple safety net that to document the restriction now. Current users > >> (core and powernv/memtrace) respect these restrictions. > > > > I do agree with the warning because it clarifies our expectations > > indeed. Se below for more questions. > > > >> Cc: Andrew Morton <akpm@linux-foundation.org> > >> Cc: Michal Hocko <mhocko@suse.com> > >> Cc: Wei Yang <richard.weiyang@linux.alibaba.com> > >> Cc: Baoquan He <bhe@redhat.com> > >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > >> Cc: Oscar Salvador <osalvador@suse.de> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> mm/memory_hotplug.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > >> index c781d386d87f9..6856702af68d9 100644 > >> --- a/mm/memory_hotplug.c > >> +++ b/mm/memory_hotplug.c > >> @@ -801,6 +801,11 @@ 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) */ > >> + if (WARN_ON_ONCE(!nr_pages || > >> + !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION))) > >> + return -EINVAL; > > > > This looks looks unnecessarily cryptic to me. Do you want to catch full > > section operation that doesn't start at the usual section boundary? If > > yes the above doesn't work work unless I am missing something. > > > > Why don't you simply WARN_ON_ONCE(nr_pages % PAGES_PER_SECTION). > > !nr_pages doesn't sound like something interesting to care about or why > > do we care? > > > > Also the start pfn has to be section aligned, so we always cover fully > aligned sections (e.g., not two partial ones). OK, I've misread your intention. I thought that we check for the start pfn prior to this warning but we only do that after. Acked-by: Michal Hocko <mhocko@suse.com>
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index c781d386d87f9..6856702af68d9 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -801,6 +801,11 @@ 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) */ + if (WARN_ON_ONCE(!nr_pages || + !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION))) + return -EINVAL; + mem_hotplug_begin(); /* associate pfn range with the zone */ @@ -1483,6 +1488,11 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages) struct memory_notify arg; char *reason; + /* We can only offline full sections (e.g., SECTION_IS_ONLINE) */ + if (WARN_ON_ONCE(!nr_pages || + !IS_ALIGNED(start_pfn | nr_pages, PAGES_PER_SECTION))) + return -EINVAL; + mem_hotplug_begin(); /*
Already two people (including me) tried to offline subsections, because the function looks like it can deal with it. But we really can only online/offline full sections (e.g., we can only mark full sections online/offline via SECTION_IS_ONLINE). Add a simple safety net that to document the restriction now. Current users (core and powernv/memtrace) respect these restrictions. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Michal Hocko <mhocko@suse.com> Cc: Wei Yang <richard.weiyang@linux.alibaba.com> Cc: Baoquan He <bhe@redhat.com> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> Cc: Oscar Salvador <osalvador@suse.de> Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+)