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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140506151252.GZ23991@suse.de>
Date:	Tue, 6 May 2014 16:12:52 +0100
From:	Mel Gorman <mgorman@...e.de>
To:	Vlastimil Babka <vbabka@...e.cz>
Cc:	Sasha Levin <sasha.levin@...cle.com>,
	Linux-MM <linux-mm@...ck.org>,
	Linux-FSDevel <linux-fsdevel@...r.kernel.org>,
	Johannes Weiner <hannes@...xchg.org>, Jan Kara <jack@...e.cz>,
	Michal Hocko <mhocko@...e.cz>, Hugh Dickins <hughd@...gle.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/17] mm: page_alloc: Use word-based accesses for
 get/set pageblock bitmaps

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@...e.cz>
> 

Thanks.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/