[lkml]   [2018]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [PATCH v2 1/5] powerpc/mm: Enhance 'slice' for supporting PPC32

Le 20/01/2018 à 18:56, Segher Boessenkool a écrit :
> Hi!
> On Sat, Jan 20, 2018 at 09:22:50AM +0100, christophe leroy wrote:
>>>>>>>>> On PPC32, the address space is limited to 4Gbytes, hence only the
>>>>>>>>> low
>>>>>>>>> slices will be used. As of today, the code uses
>>>>>>>>> SLICE_LOW_TOP (0x100000000ul) and compares it with addr to determine
>>>>>>>>> if addr refers to low or high space.
>>>>>>>>> On PPC32, such a (addr < SLICE_LOW_TOP) test is always false because
>>>>>>>>> 0x100000000ul degrades to 0. Therefore, the patch modifies
>>>>>>>>> SLICE_LOW_TOP to (0xfffffffful) and modifies the tests to
>>>>>>>>> (addr <= SLICE_LOW_TOP) which will then always be true on PPC32
>>>>>>>>> as addr has type 'unsigned long' while not modifying the PPC64
>>>>>>>>> behaviour.
> It should work to define SLICE_LOW_TOP as 0x100000000ull and keep
> everything else the same, no?

great, yes it works indeed.

>>> I don't think so. When I had the missing prototype, the compilation goes
>>> ok, including the final link. Which means at the end the code is not
>>> included since radix_enabled() evaluates to 0.
>>> Many many parts of the kernel are based on this assumption.
>> Segher, what is your opinion on the above ? Can we consider that a ' if
>> (nbits)' will always be compiled out when nbits is a #define constant,
>> or should we duplicate the macros as suggested in order to avoid
>> unneccessary 'if' test on platforms where 'nbits' is always not null by
>> definition ?
> Doing things like
> if (nbits)
> some_undeclared_function();
> will likely work in practice if the condition evaluates to false at
> compile time, but a) it will warn; b) it is just yuck; and c) it will
> not always work (for example, you get the wrong prototype in this case,
> not lethal here with most ABIs, but ugh).
> Just make sure to declare all functions, or define it to some empty
> thing, or #ifdeffery if you have to. There are many options, it is
> not hard, and if it means you have to pull code further apart that is
> not so bad: you get cleaner, clearer code.

Ok, if I understand well, your comment applies to the following indeed,
so you confirm the #ifdef is necessary.

--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -553,9 +553,11 @@ unsigned long hugetlb_get_unmapped_area(struct file
*file, unsigned long addr,
struct hstate *hstate = hstate_file(file);
int mmu_psize = shift_to_mmu_psize(huge_page_shift(hstate));

if (radix_enabled())
return radix__hugetlb_get_unmapped_area(file, addr, len,
pgoff, flags);
return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);

However, my question was related to another part of the current
patchset, where the functions are always refined:

On PPC32 we set:

+#define SLICE_LOW_SHIFT 28

On PPC64 we set:

#define SLICE_LOW_SHIFT 28

We define:

+#define slice_bitmap_zero(dst, nbits) \
+ do { if (nbits) bitmap_zero(dst, nbits); } while (0)

We have a function with:
slice_bitmap_zero(ret->low_slices, SLICE_NUM_LOW);
slice_bitmap_zero(ret->high_slices, SLICE_NUM_HIGH);
So the question is to find the better approach. Is the above approach
correct, including performance wise ?
Or should we define two sets of the macro slice_bitmap_zero(), one for
CONFIG_PPC32 with the 'if (nbits)' test and one for CONFIG_PPC64 without
the unnecessary test ?
Or should we avoid this macro entirely and instead do something like:

bitmap_zero(ret->low_slices, SLICE_NUM_LOW);
bitmap_zero(ret->high_slices, SLICE_NUM_HIGH);
And if we say the 'macro' approach is OK, should it be better the use a
static inline function instead ?


 \ /
  Last update: 2018-01-22 08:53    [W:0.047 / U:17.796 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site