Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Mel Gorman]

On Tue, Nov 28, 2017 at 05:19:10PM +0800, jiang.bi...@zte.com.cn wrote:
> > On Tue 28-11-17 09:49:45, Jiang Biao wrote:> > 1. Use unlikely to try to 
> > improve branch prediction. The
> > > *total_scan < 0* branch is unlikely to reach, so use unlikely.
> > >
> > > 2. Optimize *next_deferred >= scanned* condition.
> > > *next_deferred >= scanned* condition could be optimized into
> > > *next_deferred > scanned*, because when *next_deferred == scanned*,
> > > next_deferred shoud be 0, which is covered by the else branch.
> > >
> > > 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> > > could be merged into *next_deferred > scanned* to simplify the code.
> > 
> > How have you measured benefit of this patch?
> No accurate measurement for now.
> Theoretically, unlikely could improve branch prediction for unlikely branch.

In general, it only really matters for a heavily mispredicted path in a
fast path. It's not enforced very often but seeing a dedicated patch
making the change to a slow path is not very convincing.

> It's hard to measure the benefit of 2 and 3, any idea to do that enlightened 
> would be greatly appreciated. :)

Typically done using perf to check for mispredictions and showing a
reduction. It can also have icache benefits if code that is almost dead
is moved to another part of the function by the compiler reducing icache
pressure overall. Again, it only really matters in fast path.

> But it could simply code logic from coding 
> perspective???

It doesn't carry enough weight to stand on its own.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Mel Gorman]

On Tue, Nov 28, 2017 at 05:19:10PM +0800, jiang.bi...@zte.com.cn wrote:
> > On Tue 28-11-17 09:49:45, Jiang Biao wrote:> > 1. Use unlikely to try to 
> > improve branch prediction. The
> > > *total_scan < 0* branch is unlikely to reach, so use unlikely.
> > >
> > > 2. Optimize *next_deferred >= scanned* condition.
> > > *next_deferred >= scanned* condition could be optimized into
> > > *next_deferred > scanned*, because when *next_deferred == scanned*,
> > > next_deferred shoud be 0, which is covered by the else branch.
> > >
> > > 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> > > could be merged into *next_deferred > scanned* to simplify the code.
> > 
> > How have you measured benefit of this patch?
> No accurate measurement for now.
> Theoretically, unlikely could improve branch prediction for unlikely branch.

In general, it only really matters for a heavily mispredicted path in a
fast path. It's not enforced very often but seeing a dedicated patch
making the change to a slow path is not very convincing.

> It's hard to measure the benefit of 2 and 3, any idea to do that enlightened 
> would be greatly appreciated. :)

Typically done using perf to check for mispredictions and showing a
reduction. It can also have icache benefits if code that is almost dead
is moved to another part of the function by the compiler reducing icache
pressure overall. Again, it only really matters in fast path.

> But it could simply code logic from coding 
> perspective???

It doesn't carry enough weight to stand on its own.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Mel Gorman]

On Tue, Nov 28, 2017 at 09:49:45AM +0800, Jiang Biao wrote:
> 1. Use unlikely to try to improve branch prediction. The
> *total_scan < 0* branch is unlikely to reach, so use unlikely.
> 
> 2. Optimize *next_deferred >= scanned* condition.
> *next_deferred >= scanned* condition could be optimized into
> *next_deferred > scanned*, because when *next_deferred == scanned*,
> next_deferred shoud be 0, which is covered by the else branch.
> 
> 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> could be merged into *next_deferred > scanned* to simplify the code.
> 
> Signed-off-by: Jiang Biao 

These are slow paths. Do you have perf data indicating the branches are
frequently mispredicted? Do you have data showing this improves
performance?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Mel Gorman]

On Tue, Nov 28, 2017 at 09:49:45AM +0800, Jiang Biao wrote:
> 1. Use unlikely to try to improve branch prediction. The
> *total_scan < 0* branch is unlikely to reach, so use unlikely.
> 
> 2. Optimize *next_deferred >= scanned* condition.
> *next_deferred >= scanned* condition could be optimized into
> *next_deferred > scanned*, because when *next_deferred == scanned*,
> next_deferred shoud be 0, which is covered by the else branch.
> 
> 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> could be merged into *next_deferred > scanned* to simplify the code.
> 
> Signed-off-by: Jiang Biao 

These are slow paths. Do you have perf data indicating the branches are
frequently mispredicted? Do you have data showing this improves
performance?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Michal Hocko]

On Tue 28-11-17 17:19:10, jiang.bi...@zte.com.cn wrote:
> > On Tue 28-11-17 09:49:45, Jiang Biao wrote:> > 1. Use unlikely to try to 
> > improve branch prediction. The
> > > *total_scan < 0* branch is unlikely to reach, so use unlikely.
> > >
> > > 2. Optimize *next_deferred >= scanned* condition.
> > > *next_deferred >= scanned* condition could be optimized into
> > > *next_deferred > scanned*, because when *next_deferred == scanned*,
> > > next_deferred shoud be 0, which is covered by the else branch.
> > >
> > > 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> > > could be merged into *next_deferred > scanned* to simplify the code.
> > 
> > How have you measured benefit of this patch?
> No accurate measurement for now.
> Theoretically, unlikely could improve branch prediction for unlikely branch.

Yes, except that this is a slow path and I suspect that branch
prediction has minimal if at all.

> It's hard to measure the benefit of 2 and 3, any idea to do that enlightened 
> would be greatly appreciated. :) But it could simply code logic from coding 
> perspective。

Well, in general I wouldn't touch the code without a clear benefit.
Theoretical but unmeasurable changes would require a bigger benefit.
I am not saying it is wrong at all but I am not conviced your patch is
really worth merging.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Michal Hocko]

On Tue 28-11-17 17:19:10, jiang.bi...@zte.com.cn wrote:
> > On Tue 28-11-17 09:49:45, Jiang Biao wrote:> > 1. Use unlikely to try to 
> > improve branch prediction. The
> > > *total_scan < 0* branch is unlikely to reach, so use unlikely.
> > >
> > > 2. Optimize *next_deferred >= scanned* condition.
> > > *next_deferred >= scanned* condition could be optimized into
> > > *next_deferred > scanned*, because when *next_deferred == scanned*,
> > > next_deferred shoud be 0, which is covered by the else branch.
> > >
> > > 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> > > could be merged into *next_deferred > scanned* to simplify the code.
> > 
> > How have you measured benefit of this patch?
> No accurate measurement for now.
> Theoretically, unlikely could improve branch prediction for unlikely branch.

Yes, except that this is a slow path and I suspect that branch
prediction has minimal if at all.

> It's hard to measure the benefit of 2 and 3, any idea to do that enlightened 
> would be greatly appreciated. :) But it could simply code logic from coding 
> perspective。

Well, in general I wouldn't touch the code without a clear benefit.
Theoretical but unmeasurable changes would require a bigger benefit.
I am not saying it is wrong at all but I am not conviced your patch is
really worth merging.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Michal Hocko]

On Tue 28-11-17 09:49:45, Jiang Biao wrote:
> 1. Use unlikely to try to improve branch prediction. The
> *total_scan < 0* branch is unlikely to reach, so use unlikely.
> 
> 2. Optimize *next_deferred >= scanned* condition.
> *next_deferred >= scanned* condition could be optimized into
> *next_deferred > scanned*, because when *next_deferred == scanned*,
> next_deferred shoud be 0, which is covered by the else branch.
> 
> 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> could be merged into *next_deferred > scanned* to simplify the code.

How have you measured benefit of this patch?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/vmscan: try to optimize branch procedures.

[Michal Hocko]

On Tue 28-11-17 09:49:45, Jiang Biao wrote:
> 1. Use unlikely to try to improve branch prediction. The
> *total_scan < 0* branch is unlikely to reach, so use unlikely.
> 
> 2. Optimize *next_deferred >= scanned* condition.
> *next_deferred >= scanned* condition could be optimized into
> *next_deferred > scanned*, because when *next_deferred == scanned*,
> next_deferred shoud be 0, which is covered by the else branch.
> 
> 3. Merge two branch blocks into one. The *next_deferred > 0* branch
> could be merged into *next_deferred > scanned* to simplify the code.

How have you measured benefit of this patch?

-- 
Michal Hocko
SUSE Labs