Let's drop the basically unused section stuff and simplify. The logic
now matches the logic in __remove_pages().
Cc: Segher Boessenkool <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Oscar Salvador <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Wei Yang <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/memory_hotplug.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 8fe7e32dad48..1a00b5a37ef6 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
struct mhp_restrictions *restrictions)
{
+ const unsigned long end_pfn = pfn + nr_pages;
+ unsigned long cur_nr_pages;
int err;
- unsigned long nr, start_sec, end_sec;
struct vmem_altmap *altmap = restrictions->altmap;
err = check_hotplug_memory_addressable(pfn, nr_pages);
@@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
if (err)
return err;
- start_sec = pfn_to_section_nr(pfn);
- end_sec = pfn_to_section_nr(pfn + nr_pages - 1);
- for (nr = start_sec; nr <= end_sec; nr++) {
- unsigned long pfns;
-
- pfns = min(nr_pages, PAGES_PER_SECTION
- - (pfn & ~PAGE_SECTION_MASK));
- err = sparse_add_section(nid, pfn, pfns, altmap);
+ for (; pfn < end_pfn; pfn += cur_nr_pages) {
+ /* Select all remaining pages up to the next section boundary */
+ cur_nr_pages = min(end_pfn - pfn,
+ SECTION_ALIGN_UP(pfn + 1) - pfn);
+ err = sparse_add_section(nid, pfn, cur_nr_pages, altmap);
if (err)
break;
- pfn += pfns;
- nr_pages -= pfns;
cond_resched();
}
vmemmap_populate_print_last();
--
2.24.1
On 02/28/20 at 10:58am, David Hildenbrand wrote:
> Let's drop the basically unused section stuff and simplify. The logic
> now matches the logic in __remove_pages().
>
> Cc: Segher Boessenkool <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Oscar Salvador <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Baoquan He <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Wei Yang <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> mm/memory_hotplug.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 8fe7e32dad48..1a00b5a37ef6 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> struct mhp_restrictions *restrictions)
> {
> + const unsigned long end_pfn = pfn + nr_pages;
> + unsigned long cur_nr_pages;
> int err;
> - unsigned long nr, start_sec, end_sec;
> struct vmem_altmap *altmap = restrictions->altmap;
>
> err = check_hotplug_memory_addressable(pfn, nr_pages);
> @@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> if (err)
> return err;
>
> - start_sec = pfn_to_section_nr(pfn);
> - end_sec = pfn_to_section_nr(pfn + nr_pages - 1);
> - for (nr = start_sec; nr <= end_sec; nr++) {
> - unsigned long pfns;
> -
> - pfns = min(nr_pages, PAGES_PER_SECTION
> - - (pfn & ~PAGE_SECTION_MASK));
> - err = sparse_add_section(nid, pfn, pfns, altmap);
> + for (; pfn < end_pfn; pfn += cur_nr_pages) {
> + /* Select all remaining pages up to the next section boundary */
> + cur_nr_pages = min(end_pfn - pfn,
> + SECTION_ALIGN_UP(pfn + 1) - pfn);
> + err = sparse_add_section(nid, pfn, cur_nr_pages, altmap);
Honestly, I am not a big fan of this kind of code refactoring. The old
code may span seveal more lines or define several several more veriables,
but logic is clear, and no visible defect. It's hard to say how much we
can benefit from this kind of code simplifying, and reviewing it will take
people more time. While for the code style consistency with
__remove_page(), I would like to see it's merged. My personal opinion.
Reviewed-by: Baoquan He <[email protected]>
> if (err)
> break;
> - pfn += pfns;
> - nr_pages -= pfns;
> cond_resched();
> }
> vmemmap_populate_print_last();
> --
> 2.24.1
>
On 28.02.20 11:34, Baoquan He wrote:
> On 02/28/20 at 10:58am, David Hildenbrand wrote:
>> Let's drop the basically unused section stuff and simplify. The logic
>> now matches the logic in __remove_pages().
>>
>> Cc: Segher Boessenkool <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Oscar Salvador <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: Baoquan He <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Wei Yang <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>> ---
>> mm/memory_hotplug.c | 18 +++++++-----------
>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 8fe7e32dad48..1a00b5a37ef6 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
>> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> struct mhp_restrictions *restrictions)
>> {
>> + const unsigned long end_pfn = pfn + nr_pages;
>> + unsigned long cur_nr_pages;
>> int err;
>> - unsigned long nr, start_sec, end_sec;
>> struct vmem_altmap *altmap = restrictions->altmap;
>>
>> err = check_hotplug_memory_addressable(pfn, nr_pages);
>> @@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> if (err)
>> return err;
>>
>> - start_sec = pfn_to_section_nr(pfn);
>> - end_sec = pfn_to_section_nr(pfn + nr_pages - 1);
>> - for (nr = start_sec; nr <= end_sec; nr++) {
>> - unsigned long pfns;
>> -
>> - pfns = min(nr_pages, PAGES_PER_SECTION
>> - - (pfn & ~PAGE_SECTION_MASK));
>> - err = sparse_add_section(nid, pfn, pfns, altmap);
>> + for (; pfn < end_pfn; pfn += cur_nr_pages) {
>> + /* Select all remaining pages up to the next section boundary */
>> + cur_nr_pages = min(end_pfn - pfn,
>> + SECTION_ALIGN_UP(pfn + 1) - pfn);
>> + err = sparse_add_section(nid, pfn, cur_nr_pages, altmap);
>
> Honestly, I am not a big fan of this kind of code refactoring. The old
> code may span seveal more lines or define several several more veriables,
> but logic is clear, and no visible defect. It's hard to say how much we
I'm sorry, but iterating over variables and not using a single one in
the body is definitely not clean, at least IMHO. Leftover from
sub-section hotadd support.
> can benefit from this kind of code simplifying, and reviewing it will take
> people more time. While for the code style consistency with
> __remove_page(), I would like to see it's merged. My personal opinion.
Thanks!
--
Thanks,
David / dhildenb
On Fri, Feb 28, 2020 at 10:58:19AM +0100, David Hildenbrand wrote:
>Let's drop the basically unused section stuff and simplify. The logic
>now matches the logic in __remove_pages().
>
>Cc: Segher Boessenkool <[email protected]>
>Cc: Andrew Morton <[email protected]>
>Cc: Oscar Salvador <[email protected]>
>Cc: Michal Hocko <[email protected]>
>Cc: Baoquan He <[email protected]>
>Cc: Dan Williams <[email protected]>
>Cc: Wei Yang <[email protected]>
>Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Wei Yang <[email protected]>
>---
> mm/memory_hotplug.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 8fe7e32dad48..1a00b5a37ef6 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> struct mhp_restrictions *restrictions)
> {
>+ const unsigned long end_pfn = pfn + nr_pages;
>+ unsigned long cur_nr_pages;
> int err;
>- unsigned long nr, start_sec, end_sec;
> struct vmem_altmap *altmap = restrictions->altmap;
>
> err = check_hotplug_memory_addressable(pfn, nr_pages);
>@@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> if (err)
> return err;
>
>- start_sec = pfn_to_section_nr(pfn);
>- end_sec = pfn_to_section_nr(pfn + nr_pages - 1);
>- for (nr = start_sec; nr <= end_sec; nr++) {
>- unsigned long pfns;
>-
>- pfns = min(nr_pages, PAGES_PER_SECTION
>- - (pfn & ~PAGE_SECTION_MASK));
>- err = sparse_add_section(nid, pfn, pfns, altmap);
>+ for (; pfn < end_pfn; pfn += cur_nr_pages) {
>+ /* Select all remaining pages up to the next section boundary */
>+ cur_nr_pages = min(end_pfn - pfn,
>+ SECTION_ALIGN_UP(pfn + 1) - pfn);
>+ err = sparse_add_section(nid, pfn, cur_nr_pages, altmap);
> if (err)
> break;
>- pfn += pfns;
>- nr_pages -= pfns;
> cond_resched();
> }
> vmemmap_populate_print_last();
>--
>2.24.1
--
Wei Yang
Help you, Help me
On 02/28/20 at 12:14pm, David Hildenbrand wrote:
> On 28.02.20 11:34, Baoquan He wrote:
> > On 02/28/20 at 10:58am, David Hildenbrand wrote:
> >> Let's drop the basically unused section stuff and simplify. The logic
> >> now matches the logic in __remove_pages().
> >>
> >> Cc: Segher Boessenkool <[email protected]>
> >> Cc: Andrew Morton <[email protected]>
> >> Cc: Oscar Salvador <[email protected]>
> >> Cc: Michal Hocko <[email protected]>
> >> Cc: Baoquan He <[email protected]>
> >> Cc: Dan Williams <[email protected]>
> >> Cc: Wei Yang <[email protected]>
> >> Signed-off-by: David Hildenbrand <[email protected]>
> >> ---
> >> mm/memory_hotplug.c | 18 +++++++-----------
> >> 1 file changed, 7 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> index 8fe7e32dad48..1a00b5a37ef6 100644
> >> --- a/mm/memory_hotplug.c
> >> +++ b/mm/memory_hotplug.c
> >> @@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
> >> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> >> struct mhp_restrictions *restrictions)
> >> {
> >> + const unsigned long end_pfn = pfn + nr_pages;
> >> + unsigned long cur_nr_pages;
> >> int err;
> >> - unsigned long nr, start_sec, end_sec;
> >> struct vmem_altmap *altmap = restrictions->altmap;
> >>
> >> err = check_hotplug_memory_addressable(pfn, nr_pages);
> >> @@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> >> if (err)
> >> return err;
> >>
> >> - start_sec = pfn_to_section_nr(pfn);
> >> - end_sec = pfn_to_section_nr(pfn + nr_pages - 1);
> >> - for (nr = start_sec; nr <= end_sec; nr++) {
> >> - unsigned long pfns;
> >> -
> >> - pfns = min(nr_pages, PAGES_PER_SECTION
> >> - - (pfn & ~PAGE_SECTION_MASK));
> >> - err = sparse_add_section(nid, pfn, pfns, altmap);
> >> + for (; pfn < end_pfn; pfn += cur_nr_pages) {
> >> + /* Select all remaining pages up to the next section boundary */
> >> + cur_nr_pages = min(end_pfn - pfn,
> >> + SECTION_ALIGN_UP(pfn + 1) - pfn);
> >> + err = sparse_add_section(nid, pfn, cur_nr_pages, altmap);
> >
> > Honestly, I am not a big fan of this kind of code refactoring. The old
> > code may span seveal more lines or define several several more veriables,
> > but logic is clear, and no visible defect. It's hard to say how much we
>
> I'm sorry, but iterating over variables and not using a single one in
> the body is definitely not clean, at least IMHO. Leftover from
Hmm, sometime we do use iterator to loop over only, it's just not so good.
People usually have their own preferred coding style, and try to optimize to
remove the itch in heart, totally understand. I have no strong objection
to this, as long as it gets support from reviewers, it's not a bad
thing.
> sub-section hotadd support.
>
> > can benefit from this kind of code simplifying, and reviewing it will take
> > people more time. While for the code style consistency with
> > __remove_page(), I would like to see it's merged. My personal opinion.
>
> Thanks!
>
>
> --
> Thanks,
>
> David / dhildenb