| Message ID | 20191006085646.5768-11-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,01/10] mm/memunmap: Don't access uninitialized memmap in memunmap_pages() | expand |
On Sun, Oct 06, 2019 at 10:56:46AM +0200, David Hildenbrand wrote: > Let's drop the basically unused section stuff and simplify. > > Also, let's use a shorter variant to calculate the number of pages to > the next section boundary. > > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Wei Yang <richardw.yang@linux.intel.com> > Signed-off-by: David Hildenbrand <david@redhat.com> I have to confess that it took me while to wrap around my head with the new min() change, but looks ok: Reviewed-by: Oscar Salvador <osalvador@suse.de> > --- > mm/memory_hotplug.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 843481bd507d..2275240cfa10 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -490,25 +490,20 @@ static void __remove_section(unsigned long pfn, unsigned long nr_pages, > void __remove_pages(unsigned long pfn, unsigned long nr_pages, > struct vmem_altmap *altmap) > { > + const unsigned long end_pfn = pfn + nr_pages; > + unsigned long cur_nr_pages; > unsigned long map_offset = 0; > - unsigned long nr, start_sec, end_sec; > > map_offset = vmem_altmap_offset(altmap); > > if (check_pfn_span(pfn, nr_pages, "remove")) > return; > > - 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; > - > + for (; pfn < end_pfn; pfn += cur_nr_pages) { > cond_resched(); > - pfns = min(nr_pages, PAGES_PER_SECTION > - - (pfn & ~PAGE_SECTION_MASK)); > - __remove_section(pfn, pfns, map_offset, altmap); > - pfn += pfns; > - nr_pages -= pfns; > + /* Select all remaining pages up to the next section boundary */ > + cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK)); > + __remove_section(pfn, cur_nr_pages, map_offset, altmap); > map_offset = 0; > } > } > -- > 2.21.0 > >
On 04.02.20 10:46, Oscar Salvador wrote: > On Sun, Oct 06, 2019 at 10:56:46AM +0200, David Hildenbrand wrote: >> Let's drop the basically unused section stuff and simplify. >> >> Also, let's use a shorter variant to calculate the number of pages to >> the next section boundary. >> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Oscar Salvador <osalvador@suse.de> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Wei Yang <richardw.yang@linux.intel.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > I have to confess that it took me while to wrap around my head > with the new min() change, but looks ok: It's a pattern commonly used in compilers and emulators to calculate the number of bytes to the next block/alignment. (we're missing a macro (like we have ALIGN_UP/IS_ALIGNED) for that - but it's hard to come up with a good name (e.g., SIZE_TO_NEXT_ALIGN) .
On Tue, Feb 04, 2020 at 01:41:06PM +0100, David Hildenbrand wrote: > On 04.02.20 10:46, Oscar Salvador wrote: > > I have to confess that it took me while to wrap around my head > > with the new min() change, but looks ok: > > It's a pattern commonly used in compilers and emulators to calculate the > number of bytes to the next block/alignment. (we're missing a macro > (like we have ALIGN_UP/IS_ALIGNED) for that - but it's hard to come up > with a good name (e.g., SIZE_TO_NEXT_ALIGN) . You can just write the easy to understand ... ALIGN_UP(x) - x ... which is better *without* having a separate name. Does that not generate good machine code for you? Segher
On 04.02.20 14:13, Segher Boessenkool wrote: > On Tue, Feb 04, 2020 at 01:41:06PM +0100, David Hildenbrand wrote: >> On 04.02.20 10:46, Oscar Salvador wrote: >>> I have to confess that it took me while to wrap around my head >>> with the new min() change, but looks ok: >> >> It's a pattern commonly used in compilers and emulators to calculate the >> number of bytes to the next block/alignment. (we're missing a macro >> (like we have ALIGN_UP/IS_ALIGNED) for that - but it's hard to come up >> with a good name (e.g., SIZE_TO_NEXT_ALIGN) . > > You can just write the easy to understand > > ... ALIGN_UP(x) - x ... you mean ALIGN_UP(x, PAGES_PER_SECTION) - x but ... > > which is better *without* having a separate name. Does that not > generate good machine code for you? 1. There is no ALIGN_UP. "SECTION_ALIGN_UP(x) - x" would be possible 2. It would be wrong if x is already aligned. e.g., let's use 4096 for simplicity as we all know that value by heart (for both x and the block size). a) -(4096 | -4096) -> 4096 b) #define ALIGN_UP(x, a) ((x + a - 1) & -(a)) ALIGN_UP(4096, 4096) - 4096 -> 0 Not as easy as it seems ...
On Sun, Oct 06, 2019 at 10:56:46AM +0200, David Hildenbrand wrote: >Let's drop the basically unused section stuff and simplify. > >Also, let's use a shorter variant to calculate the number of pages to >the next section boundary. > >Cc: Andrew Morton <akpm@linux-foundation.org> >Cc: Oscar Salvador <osalvador@suse.de> >Cc: Michal Hocko <mhocko@suse.com> >Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >Cc: Dan Williams <dan.j.williams@intel.com> >Cc: Wei Yang <richardw.yang@linux.intel.com> >Signed-off-by: David Hildenbrand <david@redhat.com> Finally understand the code. Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
On Tue, Feb 04, 2020 at 02:38:51PM +0100, David Hildenbrand wrote: > On 04.02.20 14:13, Segher Boessenkool wrote: > > On Tue, Feb 04, 2020 at 01:41:06PM +0100, David Hildenbrand wrote: > >> It's a pattern commonly used in compilers and emulators to calculate the > >> number of bytes to the next block/alignment. (we're missing a macro > >> (like we have ALIGN_UP/IS_ALIGNED) for that - but it's hard to come up > >> with a good name (e.g., SIZE_TO_NEXT_ALIGN) . > > You can just write the easy to understand > > > > ... ALIGN_UP(x) - x ... > > you mean > > ALIGN_UP(x, PAGES_PER_SECTION) - x > > but ... > > > which is better *without* having a separate name. Does that not > > generate good machine code for you? > > 1. There is no ALIGN_UP. "SECTION_ALIGN_UP(x) - x" would be possible Erm, you started it ;-) > 2. It would be wrong if x is already aligned. > > e.g., let's use 4096 for simplicity as we all know that value by heart > (for both x and the block size). > > a) -(4096 | -4096) -> 4096 > > b) #define ALIGN_UP(x, a) ((x + a - 1) & -(a)) > > ALIGN_UP(4096, 4096) - 4096 -> 0 > > Not as easy as it seems ... If you always want to return a number >= 1, it it simply ALIGN_UP(x + 1) - x (and replace 1 by any other minimum size required). This *also* is easy to read, without having to have any details (and quirks :-/ ) of those utility functions memorised. Segher
On 05.02.20 13:51, Segher Boessenkool wrote: > On Tue, Feb 04, 2020 at 02:38:51PM +0100, David Hildenbrand wrote: >> On 04.02.20 14:13, Segher Boessenkool wrote: >>> On Tue, Feb 04, 2020 at 01:41:06PM +0100, David Hildenbrand wrote: >>>> It's a pattern commonly used in compilers and emulators to calculate the >>>> number of bytes to the next block/alignment. (we're missing a macro >>>> (like we have ALIGN_UP/IS_ALIGNED) for that - but it's hard to come up >>>> with a good name (e.g., SIZE_TO_NEXT_ALIGN) . > >>> You can just write the easy to understand >>> >>> ... ALIGN_UP(x) - x ... >> >> you mean >> >> ALIGN_UP(x, PAGES_PER_SECTION) - x >> >> but ... >> >>> which is better *without* having a separate name. Does that not >>> generate good machine code for you? >> >> 1. There is no ALIGN_UP. "SECTION_ALIGN_UP(x) - x" would be possible > > Erm, you started it ;-) Yeah, I was thinking in the wrong code base :) > >> 2. It would be wrong if x is already aligned. >> >> e.g., let's use 4096 for simplicity as we all know that value by heart >> (for both x and the block size). >> >> a) -(4096 | -4096) -> 4096 >> >> b) #define ALIGN_UP(x, a) ((x + a - 1) & -(a)) >> >> ALIGN_UP(4096, 4096) - 4096 -> 0 >> >> Not as easy as it seems ... > > If you always want to return a number >= 1, it it simply > ALIGN_UP(x + 1) - x I'm sorry to have to correct you again for some corner cases: ALIGN_UP(1, 4096) - 4096 = 0 Again, not as easy as it seems ...
> I'm sorry to have to correct you again for some corner cases: > > ALIGN_UP(1, 4096) - 4096 = 0 > > Again, not as easy as it seems ... > Eh, wait, I'm messing up things. Will double check :)
On 05.02.20 14:18, David Hildenbrand wrote: >> I'm sorry to have to correct you again for some corner cases: >> >> ALIGN_UP(1, 4096) - 4096 = 0 >> >> Again, not as easy as it seems ... >> > > Eh, wait, I'm messing up things. Will double check :) > Yes, makes sense, will send a patch and cc you. Thanks!
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 843481bd507d..2275240cfa10 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -490,25 +490,20 @@ static void __remove_section(unsigned long pfn, unsigned long nr_pages, void __remove_pages(unsigned long pfn, unsigned long nr_pages, struct vmem_altmap *altmap) { + const unsigned long end_pfn = pfn + nr_pages; + unsigned long cur_nr_pages; unsigned long map_offset = 0; - unsigned long nr, start_sec, end_sec; map_offset = vmem_altmap_offset(altmap); if (check_pfn_span(pfn, nr_pages, "remove")) return; - 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; - + for (; pfn < end_pfn; pfn += cur_nr_pages) { cond_resched(); - pfns = min(nr_pages, PAGES_PER_SECTION - - (pfn & ~PAGE_SECTION_MASK)); - __remove_section(pfn, pfns, map_offset, altmap); - pfn += pfns; - nr_pages -= pfns; + /* Select all remaining pages up to the next section boundary */ + cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK)); + __remove_section(pfn, cur_nr_pages, map_offset, altmap); map_offset = 0; } }
Let's drop the basically unused section stuff and simplify. Also, let's use a shorter variant to calculate the number of pages to the next section boundary. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Oscar Salvador <osalvador@suse.de> Cc: Michal Hocko <mhocko@suse.com> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Wei Yang <richardw.yang@linux.intel.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)