Re: [PATCH 08/17] mm: page_alloc: Use word-based accesses for get/set pageblock bitmaps

[Mel Gorman <mgorman@suse.de>]

On Tue, May 06, 2014 at 04:42:18PM +0200, Vlastimil Babka wrote:
> >>>+unsigned long get_pageblock_flags_mask(struct page *page,
> >>>+					unsigned long end_bitidx,
> >>>+					unsigned long nr_flag_bits,
> >>>+					unsigned long mask)
> >>>  {
> >>>  	struct zone *zone;
> >>>  	unsigned long *bitmap;
> >>>-	unsigned long pfn, bitidx;
> >>>-	unsigned long flags = 0;
> >>>-	unsigned long value = 1;
> >>>+	unsigned long pfn, bitidx, word_bitidx;
> >>>+	unsigned long word;
> >>>
> >>>  	zone = page_zone(page);
> >>>  	pfn = page_to_pfn(page);
> >>>  	bitmap = get_pageblock_bitmap(zone, pfn);
> >>>  	bitidx = pfn_to_bitidx(zone, pfn);
> >>>+	word_bitidx = bitidx / BITS_PER_LONG;
> >>>+	bitidx &= (BITS_PER_LONG-1);
> >>>
> >>>-	for (; start_bitidx <= end_bitidx; start_bitidx++, value <<= 1)
> >>>-		if (test_bit(bitidx + start_bitidx, bitmap))
> >>>-			flags |= value;
> >>>-
> >>>-	return flags;
> >>>+	word = bitmap[word_bitidx];
> >>
> >>I wonder if on some architecture this may result in inconsistent
> >>word when racing with set(), i.e. cmpxchg? We need consistency at
> >>least on the granularity of byte to prevent the problem with bogus
> >>migratetype values being read.
> >>fix:
> >
> >The number of bits align on the byte boundary so I do not think there is
> >a problem there. There is a BUILD_BUG_ON check in set_pageblock_flags_mask
> >in case this changes so it can be revisited if necessary.
> 
> I was wondering about hardware guarantees in that case (e.g.
> consistency at least on the granularity of byte when a simple memory
> read races with write) but after some discussion in the office I
> understand that hardware without such guarantees wouldn't be able to
> run Linux anyway :)
> 
> Still I wonder if ACCESS_ONCE would be safer in the 'word' variable
> assignment to protect against compiler trying to be too smart?
> 

I couldn't see a case in the get path where it would matter. I put an
ACCESS_ONCE in the set path in case the compiler accidentally determined
that old_word was invariant in that loop.

> Anyway with the nr_flag_bits removed:
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 

Thanks.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>