Hugh noted that task_capc() could use unlikely(), as most of the time there is
no capture in progress and we are in page freeing hot path. Indeed adding
unlikely() redirects produces assembly that better matches the assumption and
moves all the tests away from the hot path.
I have also noticed that we don't need to test for cc->direct_compaction as the
only place we set current->task_capture is compact_zone_order() which also
always sets cc->direct_compaction true.
Suggested-by: Hugh Dickins <[email protected]>
Signed-off-by: Vlastimil Babka <[email protected]>
---
mm/page_alloc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 48eb0f1410d4..8a4e342d7e8f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -813,11 +813,10 @@ static inline struct capture_control *task_capc(struct zone *zone)
{
struct capture_control *capc = current->capture_control;
- return capc &&
+ return unlikely(capc &&
!(current->flags & PF_KTHREAD) &&
!capc->page &&
- capc->cc->zone == zone &&
- capc->cc->direct_compaction ? capc : NULL;
+ capc->cc->zone == zone) ? capc : NULL;
}
static inline bool
--
2.27.0
On Tue, 16 Jun 2020, Vlastimil Babka wrote:
> Hugh noted that task_capc() could use unlikely(), as most of the time there is
> no capture in progress and we are in page freeing hot path. Indeed adding
> unlikely() redirects produces assembly that better matches the assumption and
> moves all the tests away from the hot path.
>
> I have also noticed that we don't need to test for cc->direct_compaction as the
> only place we set current->task_capture is compact_zone_order() which also
> always sets cc->direct_compaction true.
>
> Suggested-by: Hugh Dickins <[email protected]>
> Signed-off-by: Vlastimil Babka <[email protected]>
Acked-by: Hugh Dickins <hughd@googlecom>
Thanks for pursuing these, Vlastimil: I'm glad you were able
to remove a test and branch instead of adding one as I had.
One little thing, you've probably gone into this yourself and know
what you've written here is optimal: but I'd rather imagined it with
"unlikely(capc) && ..." instead of "unlikely(capc && ...)" - no need
to respond, please just give it a moment's consideration, Acked anyway.
> ---
> mm/page_alloc.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 48eb0f1410d4..8a4e342d7e8f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -813,11 +813,10 @@ static inline struct capture_control *task_capc(struct zone *zone)
> {
> struct capture_control *capc = current->capture_control;
>
> - return capc &&
> + return unlikely(capc &&
> !(current->flags & PF_KTHREAD) &&
> !capc->page &&
> - capc->cc->zone == zone &&
> - capc->cc->direct_compaction ? capc : NULL;
> + capc->cc->zone == zone) ? capc : NULL;
> }
>
> static inline bool
> --
> 2.27.0
>
>
On 6/16/20 10:29 PM, Hugh Dickins wrote:
> On Tue, 16 Jun 2020, Vlastimil Babka wrote:
>
>> Hugh noted that task_capc() could use unlikely(), as most of the time there is
>> no capture in progress and we are in page freeing hot path. Indeed adding
>> unlikely() redirects produces assembly that better matches the assumption and
>> moves all the tests away from the hot path.
>>
>> I have also noticed that we don't need to test for cc->direct_compaction as the
>> only place we set current->task_capture is compact_zone_order() which also
>> always sets cc->direct_compaction true.
>>
>> Suggested-by: Hugh Dickins <[email protected]>
>> Signed-off-by: Vlastimil Babka <[email protected]>
>
> Acked-by: Hugh Dickins <hughd@googlecom>
Thanks.
> Thanks for pursuing these, Vlastimil: I'm glad you were able
> to remove a test and branch instead of adding one as I had.
>
> One little thing, you've probably gone into this yourself and know
> what you've written here is optimal: but I'd rather imagined it with
> "unlikely(capc) && ..." instead of "unlikely(capc && ...)" - no need
> to respond, please just give it a moment's consideration, Acked anyway.
It makes no difference, at least on my gcc10 which seems to be smart enough to
do the right thing. But yeah, your suggestion is more readable and precise and
maybe can work better with a less smart compiler. Thanks.
----8<----
From 615eea6f6abe288ffb708aa0d1bdfbeaf30a4cbd Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <[email protected]>
Date: Tue, 16 Jun 2020 10:14:47 +0200
Subject: [PATCH] mm, page_alloc: use unlikely() in task_capc()
Hugh noted that task_capc() could use unlikely(), as most of the time there is
no capture in progress and we are in page freeing hot path. Indeed adding
unlikely() produces assembly that better matches the assumption and moves
all the tests away from the hot path.
I have also noticed that we don't need to test for cc->direct_compaction as the
only place we set current->task_capture is compact_zone_order() which also
always sets cc->direct_compaction true.
Suggested-by: Hugh Dickins <[email protected]>
Signed-off-by: Vlastimil Babka <[email protected]>
Acked-by: Hugh Dickins <hughd@googlecom>
---
mm/page_alloc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 48eb0f1410d4..18d5aed3f97b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -813,11 +813,10 @@ static inline struct capture_control *task_capc(struct zone *zone)
{
struct capture_control *capc = current->capture_control;
- return capc &&
+ return unlikely(capc) &&
!(current->flags & PF_KTHREAD) &&
!capc->page &&
- capc->cc->zone == zone &&
- capc->cc->direct_compaction ? capc : NULL;
+ capc->cc->zone == zone ? capc : NULL;
}
static inline bool
--
2.27.0
On Wed, Jun 17, 2020 at 11:55:07AM +0200, Vlastimil Babka wrote:
> <SNIP>
>
> It makes no difference, at least on my gcc10 which seems to be smart enough to
> do the right thing. But yeah, your suggestion is more readable and precise and
> maybe can work better with a less smart compiler. Thanks.
>
> ----8<----
> From 615eea6f6abe288ffb708aa0d1bdfbeaf30a4cbd Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <[email protected]>
> Date: Tue, 16 Jun 2020 10:14:47 +0200
> Subject: [PATCH] mm, page_alloc: use unlikely() in task_capc()
>
> Hugh noted that task_capc() could use unlikely(), as most of the time there is
> no capture in progress and we are in page freeing hot path. Indeed adding
> unlikely() produces assembly that better matches the assumption and moves
> all the tests away from the hot path.
>
> I have also noticed that we don't need to test for cc->direct_compaction as the
> only place we set current->task_capture is compact_zone_order() which also
> always sets cc->direct_compaction true.
>
> Suggested-by: Hugh Dickins <[email protected]>
> Signed-off-by: Vlastimil Babka <[email protected]>
> Acked-by: Hugh Dickins <hughd@googlecom>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs