Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

[Zi Yan <zi.yan@cs.rutgers.edu>]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On 7 Feb 2017, at 7:55, Aneesh Kumar K.V wrote:

> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
>
>> On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
>>> From: Zi Yan <ziy@nvidia.com>
>>>
>>> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
>>> This can cause pmd_protnone entry not being freed.
>>>
>>> Because there are two steps in changing a pmd entry to a pmd_protnone
>>> entry. First, the pmd entry is cleared to a pmd_none entry, then,
>>> the pmd_none entry is changed into a pmd_protnone entry.
>>> The racy check, even with barrier, might only see the pmd_none entry
>>> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
>>
>> That's definately a good catch.
>>
>> But I don't agree with the solution. Taking pmd lock on each
>> zap_pmd_range() is a significant hit by scalability of the code path.
>> Yes, split ptl lock helps, but it would be nice to avoid the lock in first
>> place.
>>
>> Can we fix change_huge_pmd() instead? Is there a reason why we cannot
>> setup the pmd_protnone() atomically?
>>
>> Mel? Rik?
>>
>
> I am also trying to fixup the usage of set_pte_at on ptes that are
> valid/present (that this autonuma ptes). I guess what we are missing is a
> variant of pte update routines that can atomically update a pte without
> clearing it and that also doesn't do a tlb flush ?

I think so. The key point is to have a atomic PTE update function instead
of current two-step pte/pmd_get_clear() then set_pte/pmd_at(). We can always
add a wrapper to include TLB flush, once we have this atomic update function.

I used xchg() to replace xxx_get_clear() & set_xxx_at() in pmd_protnone(),
set_pmd_migration_entry(), and remove_pmd_migration(),
then ran my test overnight. I did not see kernel crashing nor data corruption.
So I think the atomic PTE/PMD update function works without taking locks
in zap_pmd_range().

Aneesh, in your patch of fixing PowerPC's autonuma pte problem, why didn't you
use atomic operations? Is there any limitation on PowerPC?

My question is why current kernel uses xxx_get_clear() and set_xxx_at()
in the first place? Is there any limitation I do not know?


Thanks.

--
Best Regards
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 496 bytes --]