Re: [PATCH for_v21 1/2] x86/sgx: Use SRCU to protect mm_list during reclaim

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: linux-sgx@vger.kernel.org
Subject: Re: [PATCH for_v21 1/2] x86/sgx: Use SRCU to protect mm_list during reclaim
Date: Fri, 12 Jul 2019 06:44:09 +0300	[thread overview]
Message-ID: <20190712034409.6qusjbljfexe2ogf@linux.intel.com> (raw)
In-Reply-To: <20190711212549.GO15067@linux.intel.com>

On Thu, Jul 11, 2019 at 02:25:49PM -0700, Sean Christopherson wrote:
> On Fri, Jul 12, 2019 at 12:13:07AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Jul 11, 2019 at 09:16:24AM -0700, Sean Christopherson wrote:
> > > Not taking a reference to encl_mm is not just an optimization, it's also
> > > functionally necessary and a major motivation to moving to SRCu. Putting
> > > the reference can invoke sgx_encl_mm_release(), which calls
> > > synchronize_srcu() and will deadlock if done while holding the SRCU read
> > > lock.  Not taking a reference paves the way for additional refcounting
> > > improvements that would be extremely difficult to implement when using
> > > the custom walker due to cyclical dependencies on the refcount.
> > 
> > I'm not sure I get this. The existing code does not have a call to
> > synchronize_srcu().
> 
> Does this read any better?
> 
>   Not taking a reference to encl_mm is not just an optimization, it's also
>   functionally necessary and a major motivation to moving to SRCU.  From a
>   functional perspective, putting the encl_mm reference can invoke
>   sgx_encl_mm_release(), which now calls synchronize_srcu() and thus will
>   deadlock if triggered while holding the SRCU read lock.  In terms of
>   motivation, not taking a reference paves the way for additional refcounting
>   improvements that would be extremely difficult to implement when using
>   the custom walker due to cyclical dependencies on the encl_mm's refcount.

No need to change the commit message. Was just a question. I got the
code change :-) Thanks anyway for elaborating.

>  
> > > -	if (!encl)
> > > -		return;
> > > +	lockdep_assert_held_exclusive(&mm->mmap_sem);
> > 
> > Just a question: what does it check (12:10AM too tired to check,
> > apologies)?
> 
> Asserts that the caller has done down_write(&mmap_sem), i.e. guarantees
> that sgx_encl_mm_add() cannot be called in parallel on the same mm_struct.