Re: [PATCH 2/2] mm/pageblock: remove false sharing in pageblock_flags

[Alexander Duyck]

On Sun, Aug 16, 2020 at 7:11 AM Alex Shi <alex.shi@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> 在 2020/8/16 下午12:17, Matthew Wilcox 写道:
> > On Sun, Aug 16, 2020 at 11:47:57AM +0800, Alex Shi wrote:
> >> Current pageblock_flags is only 4 bits, so it has to share a char size
> >> in cmpxchg when get set, the false sharing cause perf drop.
> >>
> >> If we incrase the bits up to 8, false sharing would gone in cmpxchg. and
> >> the only cost is half char per pageblock, which is half char per 128MB
> >> on x86, 4 chars in 1 GB.
> >
> > I don't believe this patch has that effect, mostly because it still does
> > cmpxchg() on words instead of bytes.
>
> Hi Matthew,
>
> Thank a lot for comments!
>
> Sorry, I must overlook sth, would you like point out why the cmpxchg is still
> on words after patch 1 applied?
>

I would take it one step further. You still have false sharing as the
pageblocks bits still occupy the same cacheline so you are going to
see them cache bouncing regardless.

What it seems like you are attempting to address is the fact that
multiple threads could all be attempting to update the same long
value. As I pointed out for the migrate type it seems to be protected
by the zone lock, but for compaction the skip bit doesn't have the
same protection as there are some threads using the zone lock and
others using the LRU lock. I'm still not sure it makes much of a
difference though.

> >
> > But which functions would benefit?  It seems to me this cmpxchg() is
> > only called from the set_pageblock_migratetype() morass of functions,
> > none of which are called in hot paths as far as I can make out.
> >
> > So are you just reasoning by analogy with the previous patch where you
> > have measured a performance improvement, or did you send the wrong patch,
> > or did I overlook a hot path that calls one of the pageblock migration
> > functions?
> >
>
> Uh, I am reading compaction.c and found the following commit introduced
> test_and_set_skip under a lock. It looks like the pagelock_flags setting
> has false sharing in cmpxchg. but I have no valid data on this yet.
>
> Thanks
> Alex
>
> e380bebe4771548  mm, compaction: keep migration source private to a single compaction instance
>
>                 if (!locked) {
>                         locked = compact_trylock_irqsave(zone_lru_lock(zone),
>                                                                 &flags, cc);
> -                       if (!locked)
> +
> +                       /* Allow future scanning if the lock is contended */
> +                       if (!locked) {
> +                               clear_pageblock_skip(page);
>                                 break;
> +                       }
> +
> +                       /* Try get exclusive access under lock */
> +                       if (!skip_updated) {
> +                               skip_updated = true;
> +                               if (test_and_set_skip(cc, page, low_pfn))
> +                                       goto isolate_abort;
> +                       }
>

I'm not sure that is a good grounds for doubling the size of the
pageblock flags. If you look further down in the code there are bits
that are setting these bits without taking the lock. The assumption
here is that by taking the lock the test_and_set_skip will be
performed atomically since another thread cannot perform that while
the zone lock is held. If you look in the function itself it only does
anything if the skip bits are checked and if the page is the first
page in the pageblock.

I think you might be confusing some of my earlier comments. I still
believe the 3% regression you reported with my patch is not directly
related to the test_and_set_skip as the test you ran seems unlikely to
trigger compaction. However with that said one of the advantages of
using the locked section to perform these types of tests is that it
reduces the number of times the test is run since it will only be on
the first unlocked page in any batch of pages and the first page in
the pageblock is always going to be handled without the lock held
since it is the first page processed.

Until we can get a test up such as thpscale that does a good job of
stressing the compaction code I don't think we can rely on just
observations to say if this is an improvement or not.

Thanks.

- Alex