Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757206AbbEVUsm (ORCPT ); Fri, 22 May 2015 16:48:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42590 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756920AbbEVUsk (ORCPT ); Fri, 22 May 2015 16:48:40 -0400 Date: Fri, 22 May 2015 22:48:09 +0200 From: Andrea Arcangeli To: Andrew Morton Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, qemu-devel@nongnu.org, kvm@vger.kernel.org, linux-api@vger.kernel.org, Pavel Emelyanov , Sanidhya Kashyap , zhang.zhanghailiang@huawei.com, Linus Torvalds , "Kirill A. Shutemov" , Andres Lagar-Cavilla , Dave Hansen , Paolo Bonzini , Rik van Riel , Mel Gorman , Andy Lutomirski , Hugh Dickins , Peter Feiner , "Dr. David Alan Gilbert" , Johannes Weiner , "Huangpeng (Peter)" Subject: Re: [PATCH 22/23] userfaultfd: avoid mmap_sem read recursion in mcopy_atomic 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> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2620 Lines: 63 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/