Re: [PATCH v9 03/12] mm: deferred_init_memmap improvements

From: Pasha Tatashin
Date: Tue Oct 03 2017 - 12:02:55 EST

Hi Michal,

Are you OK, if I replace DEFERRED_FREE() macro with a function like this:

* Helper for deferred_init_range, free the given range, and reset the
* counters
static inline unsigned long __def_free(unsigned long *nr_free,
unsigned long *free_base_pfn,
struct page **page)
unsigned long nr = *nr_free;

deferred_free_range(*free_base_pfn, nr);
*free_base_pfn = 0;
*nr_free = 0;
*page = NULL;

return nr;

Since it is inline, and we operate with non-volatile counters, compiler will be smart enough to remove all the unnecessary de-references. As a plus, we won't be adding any new branches, and the code is still going to stay compact.


On 10/03/2017 11:15 AM, Pasha Tatashin wrote:
Hi Michal,

Please be explicit that this is possible only because we discard
memblock data later after 3010f876500f ("mm: discard memblock data
later"). Also be more explicit how the new code works.


I like how the resulting code is more compact and smaller.

That was the goal :)

for_each_free_mem_range also looks more appropriate but I really detest
the DEFERRED_FREE thingy. Maybe we can handle all that in a single goto
section. I know this is not an art but manipulating variables from
macros is more error prone and much more ugly IMHO.

Sure, I can re-arrange to have a goto place. Function won't be as small, and if compiler is not smart enough we might end up with having more branches than what my current code has.

please do not use macros. Btw. this deserves its own fix. I suspect that
purely from the review point of view it should be its own patch.

Sure, I will submit this patch separately from the rest of the project. In my opinion DEFERRED_STRUCT_PAGE_INIT is the way of the future, so we should make sure it is working with as many configs as possible.

Thank you,