From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59848) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yvtrw-0007LC-Om for qemu-devel@nongnu.org; Fri, 22 May 2015 16:48:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yvtrt-0004SD-W5 for qemu-devel@nongnu.org; Fri, 22 May 2015 16:48:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41086) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yvtrt-0004S0-Ol for qemu-devel@nongnu.org; Fri, 22 May 2015 16:48:33 -0400 Date: Fri, 22 May 2015 22:48:09 +0200 From: Andrea Arcangeli Message-ID: <20150522204809.GB4251@redhat.com> References: <1431624680-20153-1-git-send-email-aarcange@redhat.com> <1431624680-20153-23-git-send-email-aarcange@redhat.com> <20150522131822.74f374dd5a75a0285577c714@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150522131822.74f374dd5a75a0285577c714@linux-foundation.org> Subject: Re: [Qemu-devel] [PATCH 22/23] userfaultfd: avoid mmap_sem read recursion in mcopy_atomic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Morton Cc: Hugh Dickins , zhang.zhanghailiang@huawei.com, kvm@vger.kernel.org, Pavel Emelyanov , linux-api@vger.kernel.org, Johannes Weiner , Dave Hansen , linux-kernel@vger.kernel.org, qemu-devel@nongnu.org, linux-mm@kvack.org, Andres Lagar-Cavilla , Mel Gorman , Paolo Bonzini , "Kirill A. Shutemov" , "Huangpeng (Peter)" , Sanidhya Kashyap , Linus Torvalds , Andy Lutomirski , "Dr. David Alan Gilbert" , Peter Feiner On Fri, May 22, 2015 at 01:18:22PM -0700, Andrew Morton wrote: > On Thu, 14 May 2015 19:31:19 +0200 Andrea Arcangeli wrote: > > > If the rwsem starves writers it wasn't strictly a bug but lockdep > > doesn't like it and this avoids depending on lowlevel implementation > > details of the lock. > > > > ... > > > > @@ -229,13 +246,33 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm, > > > > if (!zeropage) > > err = mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma, > > - dst_addr, src_addr); > > + dst_addr, src_addr, &page); > > else > > err = mfill_zeropage_pte(dst_mm, dst_pmd, dst_vma, > > dst_addr); > > > > cond_resched(); > > > > + if (unlikely(err == -EFAULT)) { > > + void *page_kaddr; > > + > > + BUILD_BUG_ON(zeropage); > > I'm not sure what this is trying to do. BUILD_BUG_ON(local_variable)? > > It goes bang in my build. I'll just delete it. Yes, it has to be a false positive failure, so it's fine to drop it. My gcc 4.8.4 can go inside the static called function and see that only mcopy_atomic_pte can return -EFAULT. RHEL7 (4.8.3) gcc didn't complain either. Perhaps to make the BUILD_BUG_ON work with older gcc, it requrires a local variable set explicitly in the callee, but it's not worth it. It would be bad if we end up in the -EFAULT path in the zeropage case (if somebody later adds an apparently innocent -EFAULT retval and unexpectedly ends up in the mcopy_atomic_pte retry logic), but it's not important, the caller should be reviewed before improvising new retvals anyway. The retry loop addition and the BUILD_BUG_ON is all about the copy_from_user run while we already hold the mmap_sem (potentially of a different process in the non-cooperative case but it's a problem if it's the current task mmap_sem in case the rwlock implementation changes to avoid write starvation and becomes non-reentrant). lockdep definitely complains (even if I think in practice it'd be safe to read-lock recurse, we just got lockdep complains never deadlocks in fact). I didn't want to call gup_fast as copy_from_user is faster and I got an usable user mapping with likely TLB entry hot too. The lockdep warnings we hit I think were associated with NUMA hinting faults or something infrequent like that, the fast path doesn't need to retry. Thanks, Andrea