On 21.11.24 13:24, Zhen Ni wrote:
1.Removing redundant checks for current->mempolicy, with a more concise
check order.
2.Using READ_ONCE(current->mempolicy) for safe, single access to
current->mempolicy to prevent potential race conditions.
3.Optimizing the scope of task_lock(current). The lock now only protects
the critical section where mempolicy is accessed, reducing the duration
the lock is held. This enhances performance by limiting the scope of the
lock to only what is necessary.
Signed-off-by: Zhen Ni <zhen.ni@xxxxxxxxxxxx>
---
mm/mempolicy.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b646fab3e45e..8bff8830b7e6 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2132,11 +2132,14 @@ bool init_nodemask_of_mempolicy(nodemask_t *mask)
{
struct mempolicy *mempolicy;
- if (!(mask && current->mempolicy))
> + if (!mask)> + return false;
+
+ mempolicy = READ_ONCE(current->mempolicy);
+ if (!mempolicy)
return false;
task_lock(current);
- mempolicy = current->mempolicy;
switch (mempolicy->mode) {
case MPOL_PREFERRED:
case MPOL_PREFERRED_MANY:
(a) current->mempolicy can only change under the task_lock(), see
do_set_mempolicy().
(b) current->mempolicy cannot usually NULL once it was none-NULL. There
are two exceptions:
copy_process() might set it to NULL when creating the process, before
the task gets scheduled.
do_exit() might set it to NULL via mpol_put_task_policy().
I don't think init_nodemask_of_mempolicy() can be called while our task
is exiting ...
Now, if you read current->mempolicy outside of task_lock() to the
*dereference* it when it might already have changed+was freed, that's a
BUG you're introducing.
So, regarding your (1): which redundant "check" ? There is a single "not
NULL" chech. Reagrding your (2), I think you are introducing a BUG and
regrding your (3) please share performance numbers, or realize that youa
re making something up ;)
The only improvement we could make here is converting:
if (!(mask && current->mempolicy))
into a more readable
if (!mask || !current->mapping)
And maybe adding a comment that while we can race with someone changing
current->mapping, we cannot race with someone changing it to NULL.
--
Cheers,
David / dhildenb
