* [PATCH v2] sched, mm: Optimize current_gfp_context()
@ 2020-06-18 21:29 Waiman Long
2020-08-11 22:29 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Waiman Long @ 2020-06-18 21:29 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Andrew Morton, Mathieu Desnoyers,
Michel Lespinasse
Cc: linux-kernel, Waiman Long
The current_gfp_context() converts a number of PF_MEMALLOC_* per-process
flags into the corresponding GFP_* flags for memory allocation. In
that function, current->flags is accessed 3 times. That may lead to
duplicated access of the same memory location.
This is not usually a problem with minimal debug config options on as the
compiler can optimize away the duplicated memory accesses. With most
of the debug config options on, however, that may not be the case.
For example, the x86-64 object size of the __need_fs_reclaim() in a
debug kernel that calls current_gfp_context() was 309 bytes. With this
patch applied, the object size is reduced to 202 bytes. This is a saving
of 107 bytes and will probably be slightly faster too.
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/linux/sched/mm.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 480a4d1b7dd8..ff37cd8bbd82 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -181,18 +181,20 @@ static inline bool in_vfork(struct task_struct *tsk)
*/
static inline gfp_t current_gfp_context(gfp_t flags)
{
- if (unlikely(current->flags &
+ unsigned int pflags = READ_ONCE(current->flags);
+
+ if (unlikely(pflags &
(PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS | PF_MEMALLOC_NOCMA))) {
/*
* NOIO implies both NOIO and NOFS and it is a weaker context
* so always make sure it makes precedence
*/
- if (current->flags & PF_MEMALLOC_NOIO)
+ if (pflags & PF_MEMALLOC_NOIO)
flags &= ~(__GFP_IO | __GFP_FS);
- else if (current->flags & PF_MEMALLOC_NOFS)
+ else if (pflags & PF_MEMALLOC_NOFS)
flags &= ~__GFP_FS;
#ifdef CONFIG_CMA
- if (current->flags & PF_MEMALLOC_NOCMA)
+ if (pflags & PF_MEMALLOC_NOCMA)
flags &= ~__GFP_MOVABLE;
#endif
}
--
2.18.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched, mm: Optimize current_gfp_context()
2020-06-18 21:29 [PATCH v2] sched, mm: Optimize current_gfp_context() Waiman Long
@ 2020-08-11 22:29 ` Andrew Morton
2020-08-11 23:32 ` Waiman Long
2020-09-18 6:44 ` Xu, Yanfei
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2020-08-11 22:29 UTC (permalink / raw)
To: Waiman Long
Cc: Peter Zijlstra, Ingo Molnar, Mathieu Desnoyers,
Michel Lespinasse, linux-kernel
On Thu, 18 Jun 2020 17:29:36 -0400 Waiman Long <longman@redhat.com> wrote:
> The current_gfp_context() converts a number of PF_MEMALLOC_* per-process
> flags into the corresponding GFP_* flags for memory allocation. In
> that function, current->flags is accessed 3 times. That may lead to
> duplicated access of the same memory location.
>
> This is not usually a problem with minimal debug config options on as the
> compiler can optimize away the duplicated memory accesses. With most
> of the debug config options on, however, that may not be the case.
> For example, the x86-64 object size of the __need_fs_reclaim() in a
> debug kernel that calls current_gfp_context() was 309 bytes. With this
> patch applied, the object size is reduced to 202 bytes. This is a saving
> of 107 bytes and will probably be slightly faster too.
>
> ...
>
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -181,18 +181,20 @@ static inline bool in_vfork(struct task_struct *tsk)
> */
> static inline gfp_t current_gfp_context(gfp_t flags)
> {
> - if (unlikely(current->flags &
> + unsigned int pflags = READ_ONCE(current->flags);
Why use READ_ONCE() here?
> + if (unlikely(pflags &
> (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS | PF_MEMALLOC_NOCMA))) {
> /*
> * NOIO implies both NOIO and NOFS and it is a weaker context
> * so always make sure it makes precedence
> */
> - if (current->flags & PF_MEMALLOC_NOIO)
> + if (pflags & PF_MEMALLOC_NOIO)
> flags &= ~(__GFP_IO | __GFP_FS);
> - else if (current->flags & PF_MEMALLOC_NOFS)
> + else if (pflags & PF_MEMALLOC_NOFS)
> flags &= ~__GFP_FS;
> #ifdef CONFIG_CMA
> - if (current->flags & PF_MEMALLOC_NOCMA)
> + if (pflags & PF_MEMALLOC_NOCMA)
> flags &= ~__GFP_MOVABLE;
> #endif
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched, mm: Optimize current_gfp_context()
2020-08-11 22:29 ` Andrew Morton
@ 2020-08-11 23:32 ` Waiman Long
2020-09-18 6:44 ` Xu, Yanfei
1 sibling, 0 replies; 6+ messages in thread
From: Waiman Long @ 2020-08-11 23:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Zijlstra, Ingo Molnar, Mathieu Desnoyers,
Michel Lespinasse, linux-kernel
On 8/11/20 6:29 PM, Andrew Morton wrote:
> On Thu, 18 Jun 2020 17:29:36 -0400 Waiman Long <longman@redhat.com> wrote:
>
>> The current_gfp_context() converts a number of PF_MEMALLOC_* per-process
>> flags into the corresponding GFP_* flags for memory allocation. In
>> that function, current->flags is accessed 3 times. That may lead to
>> duplicated access of the same memory location.
>>
>> This is not usually a problem with minimal debug config options on as the
>> compiler can optimize away the duplicated memory accesses. With most
>> of the debug config options on, however, that may not be the case.
>> For example, the x86-64 object size of the __need_fs_reclaim() in a
>> debug kernel that calls current_gfp_context() was 309 bytes. With this
>> patch applied, the object size is reduced to 202 bytes. This is a saving
>> of 107 bytes and will probably be slightly faster too.
>>
>> ...
>>
>> --- a/include/linux/sched/mm.h
>> +++ b/include/linux/sched/mm.h
>> @@ -181,18 +181,20 @@ static inline bool in_vfork(struct task_struct *tsk)
>> */
>> static inline gfp_t current_gfp_context(gfp_t flags)
>> {
>> - if (unlikely(current->flags &
>> + unsigned int pflags = READ_ONCE(current->flags);
> Why use READ_ONCE() here?
It was a change suggested by PeterZ. He said without the READ_ONCE(),
the compiler may still choose to access current->flags multiple times
instead of just reading it once. I think he is right and so I made the
change.
Cheers,
Longman
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched, mm: Optimize current_gfp_context()
2020-08-11 22:29 ` Andrew Morton
2020-08-11 23:32 ` Waiman Long
@ 2020-09-18 6:44 ` Xu, Yanfei
2020-09-18 15:18 ` Waiman Long
1 sibling, 1 reply; 6+ messages in thread
From: Xu, Yanfei @ 2020-09-18 6:44 UTC (permalink / raw)
To: Waiman Long
Cc: Andrew Morton, Peter Zijlstra, Ingo Molnar, Mathieu Desnoyers,
Michel Lespinasse, linux-kernel
Hi Waiman,
On 8/12/20 6:29 AM, Andrew Morton wrote:
> On Thu, 18 Jun 2020 17:29:36 -0400 Waiman Long <longman@redhat.com> wrote:
>
>> The current_gfp_context() converts a number of PF_MEMALLOC_* per-process
>> flags into the corresponding GFP_* flags for memory allocation. In
>> that function, current->flags is accessed 3 times. That may lead to
>> duplicated access of the same memory location.
>>
I have a puzzle about this comment, what's the meaning about "That may
lead to duplicated access of the same memory location". After using
variable 'pflags', will it not duplicated access the same memory
location?
Looking forward to your reply :)
Thanks,
Yanfei
>> This is not usually a problem with minimal debug config options on as the
>> compiler can optimize away the duplicated memory accesses. With most
>> of the debug config options on, however, that may not be the case.
>> For example, the x86-64 object size of the __need_fs_reclaim() in a
>> debug kernel that calls current_gfp_context() was 309 bytes. With this
>> patch applied, the object size is reduced to 202 bytes. This is a saving
>> of 107 bytes and will probably be slightly faster too.
>>
>> ...
>>
>> --- a/include/linux/sched/mm.h
>> +++ b/include/linux/sched/mm.h
>> @@ -181,18 +181,20 @@ static inline bool in_vfork(struct task_struct *tsk)
>> */
>> static inline gfp_t current_gfp_context(gfp_t flags)
>> {
>> - if (unlikely(current->flags &
>> + unsigned int pflags = READ_ONCE(current->flags);
>
> Why use READ_ONCE() here?
>
>> + if (unlikely(pflags &
>> (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS | PF_MEMALLOC_NOCMA))) {
>> /*
>> * NOIO implies both NOIO and NOFS and it is a weaker context
>> * so always make sure it makes precedence
>> */
>> - if (current->flags & PF_MEMALLOC_NOIO)
>> + if (pflags & PF_MEMALLOC_NOIO)
>> flags &= ~(__GFP_IO | __GFP_FS);
>> - else if (current->flags & PF_MEMALLOC_NOFS)
>> + else if (pflags & PF_MEMALLOC_NOFS)
>> flags &= ~__GFP_FS;
>> #ifdef CONFIG_CMA
>> - if (current->flags & PF_MEMALLOC_NOCMA)
>> + if (pflags & PF_MEMALLOC_NOCMA)
>> flags &= ~__GFP_MOVABLE;
>> #endif
>> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched, mm: Optimize current_gfp_context()
2020-09-18 6:44 ` Xu, Yanfei
@ 2020-09-18 15:18 ` Waiman Long
2020-09-19 14:55 ` Xu, Yanfei
0 siblings, 1 reply; 6+ messages in thread
From: Waiman Long @ 2020-09-18 15:18 UTC (permalink / raw)
To: Xu, Yanfei
Cc: Andrew Morton, Peter Zijlstra, Ingo Molnar, Mathieu Desnoyers,
Michel Lespinasse, linux-kernel
On 9/18/20 2:44 AM, Xu, Yanfei wrote:
> Hi Waiman,
>
> On 8/12/20 6:29 AM, Andrew Morton wrote:
>> On Thu, 18 Jun 2020 17:29:36 -0400 Waiman Long <longman@redhat.com>
>> wrote:
>>
>>> The current_gfp_context() converts a number of PF_MEMALLOC_*
>>> per-process
>>> flags into the corresponding GFP_* flags for memory allocation. In
>>> that function, current->flags is accessed 3 times. That may lead to
>>> duplicated access of the same memory location.
>>>
> I have a puzzle about this comment, what's the meaning about "That may
> lead to duplicated access of the same memory location". After using
> variable 'pflags', will it not duplicated access the same memory
> location?
> Looking forward to your reply :)
That condition usually won't happen on a non-debug kernel. However, if
certain debugging capability is turned on, access to current will be
compiled into a bunch of checks and memory accesses. So if current is
used multiple times, the same set of codes will be duplicated the same
number of times slowing down the operation and increasing code size. By
accessing current once, we avoid this overhead in a debug kernel.
Cheers,
Longman
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched, mm: Optimize current_gfp_context()
2020-09-18 15:18 ` Waiman Long
@ 2020-09-19 14:55 ` Xu, Yanfei
0 siblings, 0 replies; 6+ messages in thread
From: Xu, Yanfei @ 2020-09-19 14:55 UTC (permalink / raw)
To: Waiman Long
Cc: Andrew Morton, Peter Zijlstra, Ingo Molnar, Mathieu Desnoyers,
Michel Lespinasse, linux-kernel
On 9/18/20 11:18 PM, Waiman Long wrote:
> On 9/18/20 2:44 AM, Xu, Yanfei wrote:
>> Hi Waiman,
>>
>> On 8/12/20 6:29 AM, Andrew Morton wrote:
>>> On Thu, 18 Jun 2020 17:29:36 -0400 Waiman Long <longman@redhat.com>
>>> wrote:
>>>
>>>> The current_gfp_context() converts a number of PF_MEMALLOC_*
>>>> per-process
>>>> flags into the corresponding GFP_* flags for memory allocation. In
>>>> that function, current->flags is accessed 3 times. That may lead to
>>>> duplicated access of the same memory location.
>>>>
>> I have a puzzle about this comment, what's the meaning about "That may
>> lead to duplicated access of the same memory location". After using
>> variable 'pflags', will it not duplicated access the same memory
>> location?
>> Looking forward to your reply :)
>
> That condition usually won't happen on a non-debug kernel. However, if
> certain debugging capability is turned on, access to current will be
> compiled into a bunch of checks and memory accesses. So if current is
> used multiple times, the same set of codes will be duplicated the same
> number of times slowing down the operation and increasing code size. By
> accessing current once, we avoid this overhead in a debug kernel.
>
> Cheers,
> Longman
>
Hah， got it.
Thanks for your detailed explain！
cheers，
Yanfei
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-19 14:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-18 21:29 [PATCH v2] sched, mm: Optimize current_gfp_context() Waiman Long
2020-08-11 22:29 ` Andrew Morton
2020-08-11 23:32 ` Waiman Long
2020-09-18 6:44 ` Xu, Yanfei
2020-09-18 15:18 ` Waiman Long
2020-09-19 14:55 ` Xu, Yanfei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).