Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] hugetlbfs: Take read_lock on i_mmap for PMD sharing
To:     Matthew Wilcox <willy@infradead.org>,
        Waiman Long <longman@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Will Deacon <will.deacon@arm.com>
References: <20191107190628.22667-1-longman@redhat.com>
 <20191107195441.GF11823@bombadil.infradead.org>
 <ed46ef09-7766-eb80-a4ad-4c72d8dba188@oracle.com>
 <20191108020456.sulyjskhq3s5zcaa@linux-p48b>
From:   Mike Kravetz <mike.kravetz@oracle.com>
Message-ID: <ea057d15-5205-9992-af95-b2727df577c4@oracle.com>
Date:   Fri, 8 Nov 2019 11:10:22 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <20191108020456.sulyjskhq3s5zcaa@linux-p48b>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/7/19 6:04 PM, Davidlohr Bueso wrote:
> On Thu, 07 Nov 2019, Mike Kravetz wrote:
> 
>> Note that huge_pmd_share now increments the page count with the semaphore
>> held just in read mode.  It is OK to do increments in parallel without
>> synchronization.  However, we don't want anyone else changing the count
>> while that check in huge_pmd_unshare is happening.  Hence, the need for
>> taking the semaphore in write mode.
> 
> This would be a nice addition to the changelog methinks.

Last night I remembered there is one place where we currently take
i_mmap_rwsem in read mode and potentially call huge_pmd_unshare.  That
is in try_to_unmap_one.  Yes, there is a potential race here today.
But that race is somewhat contained as you need two threads doing some
combination of page migration and page poisoning to race.  This change
now allows migration or poisoning to race with page fault.  I would
really prefer if we do not open up the race window in this manner.

Getting this right in the try_to_unmap_one case is a bit tricky.  I had
code to do this in the past that was part of a bigger hugetlb synchronization
change.  All those changes got reverted (commit ddeaab32a89f), but I
believe it is possible to change try_to_unmap_one calling sequences
without introducing other issues.

Bottom line is that more changes are needed in this patch.  I'll work
on those changes unless someone else volunteers.  It will likely take me
one or two days to come up with and test proposed changes.
-- 
Mike Kravetz