On 16.10.19 17:31, Anshuman Khandual wrote:
On 10/16/2019 06:11 PM, Michal Hocko wrote:
On Wed 16-10-19 14:29:05, David Hildenbrand wrote:
On 16.10.19 13:51, Michal Hocko wrote:
On Wed 16-10-19 16:43:57, Anshuman Khandual wrote:
On 10/16/2019 04:39 PM, David Hildenbrand wrote:
[...]
Just to make sure, you ignored my comment regarding alignment
although I explicitly mentioned it a second time? Thanks.
I had asked Michal explicitly what to be included for the respin. Anyways
seems like the previous thread is active again. I am happy to incorporate
anything new getting agreed on there.
Your patch is using the same alignment as the original code would do. If
an explicit alignement is needed then this can be added on top, right?
Again, the "issue" I see here is that we could now pass in numbers that are
not a power of two. For gigantic pages it was clear that we always have a
number of two. The alignment does not make any sense otherwise.
ALIGN() does expect nr_pages two be power of two otherwise the mask
value might not be correct, affecting start pfn value for a zone.
#define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
#define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
What I'm asking for is
a) Document "The resulting PFN is aligned to nr_pages" and "nr_pages should
be a power of two".
OK, this makes sense.
Sure, will add this to the alloc_contig_pages() helper description and
in the commit message as well.
As long as it is documented that implicit alignment will happen, fine
with me.
The thing about !is_power_of2() is that we usually don't need an
alignment there (or instead an explicit one). And as I mentioned, the
current function might fail easily to allocate a suitable range due to
the way the search works (== check aligned blocks only). The search
really only provides reliable results when size==alignment and it's a
power of two IMHO. Not documenting that is in my opinion misleading -
somebody who wants !is_power_of2() and has no alignment requirements
should probably rework the function first.
So with some documentation regarding that
Acked-by: David Hildenbrand <david@xxxxxxxxxx>
--
Thanks,
David / dhildenb
