On Tue, 6 Mar 2018 15:41:00 +0100
Christophe LEROY <christophe.le...@c-s.fr> wrote:
> Le 06/03/2018 à 14:25, Nicholas Piggin a écrit :
> > +static bool slice_check_range_fits(struct mm_struct *mm,
> > + const struct slice_mask *available,
> > + unsigned long start, unsigned long len)
> > {
> > - DECLARE_BITMAP(result, SLICE_NUM_HIGH);
> > - /*
> > - * Make sure we just do bit compare only to the max
> > - * addr limit and not the full bit map size.
> > - */
> > - unsigned long slice_count =
> > GET_HIGH_SLICE_INDEX(mm->context.slb_addr_limit);
> > + unsigned long end = start + len - 1;
> > + u64 low_slices = 0;
> >
> > - if (!SLICE_NUM_HIGH)
> > - return (mask->low_slices & available->low_slices) ==
> > - mask->low_slices;
> > + if (start < SLICE_LOW_TOP) {
> > + unsigned long mend = min(end, (SLICE_LOW_TOP - 1));
>
> See slice_range_to_mask()
>
> You'll have an issue here with PPC32, you have to cast (SLICE_LOW_TOP -
> 1) to unsigned long because SLICE_LOW_TOP is unsigned long long on PPC32
Okay thanks. Forgot to cross compiled it on 8xx, so I'll do that next
time.
> > +
> > + low_slices = (1u << (GET_LOW_SLICE_INDEX(mend) + 1))
> > + - (1u << GET_LOW_SLICE_INDEX(start));
> > + }
> > + if ((low_slices & available->low_slices) != low_slices)
> > + return false;
> > +
> > + if (SLICE_NUM_HIGH && ((start + len) > SLICE_LOW_TOP)) {
> > + unsigned long start_index = GET_HIGH_SLICE_INDEX(start);
> > + unsigned long align_end = ALIGN(end, (1UL << SLICE_HIGH_SHIFT));
> > + unsigned long count = GET_HIGH_SLICE_INDEX(align_end) -
> > start_index;
> > + unsigned long i;
> >
> > - bitmap_and(result, mask->high_slices,
> > - available->high_slices, slice_count);
> > + for (i = start_index; i < start_index + count; i++) {
> > + if (!test_bit(i, available->high_slices))
> > + return false;
> > + }
>
> What about using bitmap_find_next_zero_area()
I'll look at it. Perhaps in another patch, because existing
loops are not using bitmap range operations either. A series
to convert those is a good idea.
> > @@ -562,15 +571,11 @@ unsigned long slice_get_unmapped_area(unsigned long
> > addr, unsigned long len,
> > #endif
> >
> > /* First check hint if it's valid or if we have MAP_FIXED */
> > - if (addr != 0 || fixed) {
> > - /* Build a mask for the requested range */
> > - slice_range_to_mask(addr, len, &mask);
> > - slice_print_mask(" mask", &mask);
> > -
> > + if (addr || fixed) {
>
> It is cleanup, should it really be part of this patch ?
> > @@ -596,10 +601,11 @@ unsigned long slice_get_unmapped_area(unsigned long
> > addr, unsigned long len,
> > slice_or_mask(&potential_mask, &good_mask);
> > slice_print_mask(" potential", &potential_mask);
> >
> > - if ((addr != 0 || fixed) &&
> > - slice_check_fit(mm, &mask, &potential_mask)) {
> > - slice_dbg(" fits potential !\n");
> > - goto convert;
> > + if (addr || fixed) {
> > + if (slice_check_range_fits(mm, &potential_mask, addr, len)) {
> > + slice_dbg(" fits potential !\n");
> > + goto convert;
> > + }
>
> Why not keep the original structure and just replacing slice_check_fit()
> by slice_check_range_fits() ?
>
> I believe cleanups should not be mixed with real feature changes. If
> needed, you should have a cleanup patch up front the serie.
For code that is already changing, I think minor cleanups are okay if
they're very simple. Maybe this is getting to the point of needing
another patch. You've made valid points for a lot of other unnecessary
cleanups though, so I'll fix all of those.
Thanks,
Nick
