Date:   Wed, 1 Jun 2022 14:33:02 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
CC:     Ying Huang <ying.huang@...el.com>, <peterx@...hat.com>,
        <apopple@...dia.com>, <osalvador@...e.de>,
        <mike.kravetz@...cle.com>, <songmuchun@...edance.com>,
        <hch@....de>, <dhowells@...hat.com>, <cl@...ux.com>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <akpm@...ux-foundation.org>, <naoya.horiguchi@....com>
Subject: Re: [PATCH v4 1/4] mm: reduce the rcu lock duration

On 2022/6/1 0:09, Eric W. Biederman wrote:
> Miaohe Lin <linmiaohe@...wei.com> 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@xmission.com/
>> "
> 
> 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.

> 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.

> 
> 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.

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.

> 
> Eric

Thanks!

> .
> 

Powered by blists - more mailing lists