Re: [PATCH v4 1/4] mm: reduce the rcu lock duration

[Eric W. Biederman]

Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:

> On 2022/6/1 0:09, Eric W. Biederman wrote:
>> Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:
> snip
>>>
>>> "
>>> Commit 3268c63eded4 ("mm: fix move/migrate_pages() race on task struct")
>>> extends the period of the rcu_read_lock until after the permissions checks
>>> are done because it suspects the permissions checks are not safe unless
>>> performed under both rcu_read_lock and task_lock to ensure the task<->mm
>>> association does not change on us while we are working [1]. But extended
>>> rcu read lock does not add much value. Because after permission checking
>>> the permission may still be changed. There's no much difference. So it's
>>> unnecessary to extend the period of the rcu_read_lock. Release the rcu
>>> lock after task refcount is successfully grabbed to reduce the rcu holding
>>> time.
>>>
>>> [1] https://lore.kernel.org/lkml/87sjhzun47.fsf@xxxxxxxxxxxx/
>>> "
>> 
>> It doesn't make sense to me.
>> 
>> I don't see any sleeping functions called from find_mm_struct or
>> kernel_migrate_pages in the area kernel_migrate_pages in the area of the
>> code protected by get_task_struct.  So at a very basic level I see a
>> justification for dirtying a cache line twice with get_task_struct and
>> put_task_struct to reduce rcu_read_lock hold times.
>> 
>> I would contend that a reasonable cleanup based up on the current state
>> of the code would be to extend the rcu_read_lock over get_task_mm so
>
> If so, security_task_movememory will be called inside rcu lock. It might
> call sleeping functions, e.g. smack_log(). I think it's not a good
> idea.

In general the security functions are not allowed to sleep.
The audit mechanism typically preallocates memory so it does
not need to sleep.  From a quick skim through the code I don't
see smack_log sleeping.

Certainly the security hooks are not supposed to be inserted into the
code that they prevent reasonable implementation decisions.

Which is to say if there is a good (non-security hook reason) for
supporting sleeping deal with it.  Otherwise the security hooks has a
bug and needs to get fixed/removed.

>> that a reference to task_struct does not need to be taken.  That has
>> the potential to reduce contention and reduce lock hold times.
>> 
>> 
>> The code is missing a big fat comment with the assertion that it is ok
>> if the permission checks are racy because the race is small, and the
>> worst case thing that happens is the page is migrated to another
>> numa node.
>> 
>> 
>> Given that the get_mm_task takes task_lock the cost of dirtying the
>> cache line is already being paid.  Perhaps not extending task_lock hold
>> times a little bit is justified, but I haven't seen that case made.
>> 
>> This seems like code that is called little enough it would be better for
>> it to be correct, and not need big fat comments explaining why it
>> doesn't matter that they code is deliberately buggy.
>> 
>
> Agree. A big fat comments will make code hard to follow.

No.

The code is impossible to follow currently.

The code either requires a comment pointing out that it is deliberately
racy, or the code needs to be fixed.

Clever and subtle code always requires a comment if for no other
reason then to alert the reader that something a typical is going on.

>> In short it does not make sense to me to justify a patch for performance
>> reasons when it appears that extending the rcu_read_lock hold time and
>> not touch the task reference count would stop dirtying a cache line and
>> likely have more impact.
>
> IMHO, incremented task refcount should make code works correctly. And extending
> the rcu_read_lock over get_task_mm will break the things because sleeping functions
> might be called while holding rcu lock.

Which sleeping functions?

I can't find any.  In particular smack_task_movememory calls
smk_curacc_on_task which is the same function called by
security_task_getpgid.  security_task_getpgid is called
under rcu_read_lock.  So smack won't sleep.

> Does the patch itself makes sense for you? Should I rephase the commit log further?
> I'm afraid I didn't get your point correctly.

The patch makes no sense to me because I don't see it doing anything
worth doing.

get/put_task_struct both dirty a cache line and are expensive especially
when contended.  Dirtying a cache line that is contended is the pretty
much the most expensive native cpu operation.  In pathological scenarios
I have seen it take up to 1s.  Realistically in a cache cold scenario
(which is not as bad as a contended scenario) you are looking at 100ns
or more just to execute get_task_struct/put_task_struct.  That is the
kind of cost it would be nice to avoid all together (even if the
pathological case never comes up).

So I see two pieces of code where we could use the cheap operation
rcu_read_lock/rcu_read_unlock to remove the expensive operation
get_task_struct/put_task_struct.  Instead I see people removing
rcu_read_lock/rcu_read_unlock.

That makes no sense.  Especially as your implied reason for making this
change is to make the code have better performance.  Improving
performance is the reason for making critical sections smaller isn't it?

Eric