linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Nicholas Piggin <npiggin@gmail.com>
To: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: linuxppc-dev@lists.ozlabs.org,
	Alex Williamson <alex.williamson@redhat.com>,
	Paul Mackerras <paulus@samba.org>,
	kvm@vger.kernel.org, David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH kernel v3 3/4] vfio/spapr: Cache mm in tce_container
Date: Mon, 24 Oct 2016 15:55:55 +1100	[thread overview]
Message-ID: <20161024155555.333c2f7a@roar.ozlabs.ibm.com> (raw)
In-Reply-To: <734716b1-6a77-55e1-ed9b-dff635475b0b@ozlabs.ru>

On Mon, 24 Oct 2016 15:25:34 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 20/10/16 18:31, Nicholas Piggin wrote:
> > On Thu, 20 Oct 2016 14:03:49 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >   
> >> In some situations the userspace memory context may live longer than
> >> the userspace process itself so if we need to do proper memory context
> >> cleanup, we better cache @mm and use it later when the process is gone
> >> (@current or @current->mm is NULL).
> >>
> >> This references mm and stores the pointer in the container; this is done
> >> when a container is just created so checking for !current->mm in other
> >> places becomes pointless.
> >>
> >> This replaces current->mm with container->mm everywhere except debug
> >> prints.
> >>
> >> This adds a check that current->mm is the same as the one stored in
> >> the container to prevent userspace from registering memory in other
> >> processes.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >>  drivers/vfio/vfio_iommu_spapr_tce.c | 127 ++++++++++++++++++++----------------
> >>  1 file changed, 71 insertions(+), 56 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> index d0c38b2..6b0b121 100644
> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -31,49 +31,46 @@  
> > 
> > Does it make sense to move the rest of these hunks into patch 2?
> > I think they're similarly just moving the mm reference into callers.  
> 
> 
> Patch #2 is moving chunks between 2 maintainership areas - ppc64 and vfio,
> this one changes only vfio code, usually it is easier to split patches this
> way.

Okay.


> >> -static void decrement_locked_vm(long npages)
> >> +static void decrement_locked_vm(struct mm_struct *mm, long npages)
> >>  {
> >> -	if (!current || !current->mm || !npages)
> >> +	if (!mm || !npages)
> >>  		return; /* process exited */  
> > 
> > I know you're trying to be defensive and change as little logic as possible,
> > but some cases should be an error, and I think some of the "process exited"
> > comments were wrong anyway.
> > 
> > Maybe pull the !mm test into the caller and make it WARN_ON?  
> 
> 
> No, the next patch should just drop this check as I am going to have a
> valid mm pointer in a container all its lifetime.

That works too.


> >> @@ -317,6 +311,9 @@ static void *tce_iommu_open(unsigned long arg)
> >>  		return ERR_PTR(-EINVAL);
> >>  	}
> >>  
> >> +	if (!current->mm)
> >> +		return ERR_PTR(-ESRCH); /* process exited */  
> > 
> > A userspace thread in the kernel can't have its mm disappear, unless you
> > are actually in the exit code. !current->mm is more like a test for a kernel
> > thread.  
> 
> Sorry, I am not following you here. I am going to use @mm, I need to check
> if it is not NULL for whatever reason, I do this here, once, but it is
> pointless anyway?

If you are going to use mm, and it's mm of a normal process context,
then you don't have to check if it is NULL.

This looks like you are expecting the call to be made the middle of
exit(2), which surely is not the case?


> >> @@ -326,13 +323,17 @@ static void *tce_iommu_open(unsigned long arg)
> >>  
> >>  	container->v2 = arg == VFIO_SPAPR_TCE_v2_IOMMU;
> >>  
> >> +	container->mm = current->mm;
> >> +	atomic_inc(&container->mm->mm_count);
> >> +
> >>  	return container;  
> > 
> > It's a nitpick if you respin the patch, but I guess it would better be
> > described as a reference than a cache of the object. "have tce_container
> > take a reference to mm_struct".  
> 
> Ok, will do!
> 
> 
> > 
> >   
> >> @@ -515,13 +526,16 @@ static long tce_iommu_build_v2(struct tce_container *container,
> >>  	unsigned long hpa;
> >>  	enum dma_data_direction dirtmp;
> >>  
> >> +	if (container->mm != current->mm)
> >> +		return -ESRCH;  
> > 
> > Good, is this condition now enforced on all entrypoints that use
> > container->mm (except the final teardown)? (The mlock/rlimit stuff,
> > as we talked about before, doesn't make sense if not).  
> 
> After having a chat with Paul, I'll move this check (slightly improved) to
> the beginning of tce_iommu_ioctl().

Sounds good. I'll take another look when you repost them.

Thanks,
Nick

  reply	other threads:[~2016-10-24  4:56 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-20  3:03 [PATCH kernel v3 0/4] powerpc/spapr/vfio: Put pages on VFIO container shutdown Alexey Kardashevskiy
2016-10-20  3:03 ` [PATCH kernel v3 1/4] powerpc/iommu: Pass mm_struct to init/cleanup helpers Alexey Kardashevskiy
2016-10-20 23:14   ` David Gibson
2016-10-20  3:03 ` [PATCH kernel v3 2/4] powerpc/iommu: Stop using @current in mm_iommu_xxx Alexey Kardashevskiy
2016-10-20 23:18   ` David Gibson
2016-10-20  3:03 ` [PATCH kernel v3 3/4] vfio/spapr: Cache mm in tce_container Alexey Kardashevskiy
2016-10-20  7:31   ` Nicholas Piggin
2016-10-21  0:21     ` David Gibson
2016-10-21  1:47       ` Nicholas Piggin
2016-10-24  4:25     ` Alexey Kardashevskiy
2016-10-24  4:55       ` Nicholas Piggin [this message]
2016-10-21  0:25   ` David Gibson
2016-10-20  3:03 ` [PATCH kernel v3 4/4] powerpc/mm/iommu, vfio/spapr: Put pages on VFIO container shutdown Alexey Kardashevskiy
2016-10-21  0:29   ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161024155555.333c2f7a@roar.ozlabs.ibm.com \
    --to=npiggin@gmail.com \
    --cc=aik@ozlabs.ru \
    --cc=alex.williamson@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=kvm@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).