Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Jason Gunthorpe <jgg@mellanox.com>
To: Ralph Campbell <rcampbell@nvidia.com>
Cc: "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	Jerome Glisse <jglisse@redhat.com>,
	John Hubbard <jhubbard@nvidia.com>,
	Christoph Hellwig <hch@lst.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ben Skeggs <bskeggs@redhat.com>, Shuah Khan <shuah@kernel.org>
Subject: Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers
Date: Thu, 16 Jan 2020 16:00:06 +0000
Message-ID: <20200116160002.GL20978@mellanox.com> (raw)
In-Reply-To: <5845f50e-8bc0-8068-ee21-4f910beb1255@nvidia.com>

On Wed, Jan 15, 2020 at 02:09:47PM -0800, Ralph Campbell wrote:

> I don't understand the lifetime/membership issue. The driver is the only thing
> that allocates, inserts, or removes struct mmu_interval_notifier and thus
> completely controls the lifetime.

If the returned value is on the defered list it could be freed at any
moment. The existing locks do not prevent it.

> > > +		ret = nouveau_svmm_interval_find(svmm, &range);
> > > +		if (ret) {
> > > +			up_read(&mm->mmap_sem);
> > > +			return ret;
> > > +		}
> > > +		range.notifier_seq = mmu_interval_read_begin(range.notifier);
> > >   		ret = hmm_range_fault(&range, 0);
> > >   		up_read(&mm->mmap_sem);
> > >   		if (ret <= 0) {
> > 
> > I'm still not sure this is a better approach than what ODP does. It
> > looks very expensive on the fault path..
> > 
> > Jason
> > 
> 
> ODP doesn't have this problem because users have to call ib_reg_mr()
> before any I/O can happen to the process address space.

ODP supports a single 'full VA' call at process startup, just like
these cases.

> That is when mmu_interval_notifier_insert() /
> mmu_interval_notifier_remove() can be called and the driver doesn't
> have to worry about the interval changing sizes or being removed
> while I/O is happening.  

No, for the 'ODP full process VA' (aka implicit ODP) mode it
dynamically maintains a list of intervals. ODP chooses the align the
dynamic intervals to it's HW page table levels, and not to SW VMAs.
This is much simpler to manage and faster to fault, at the cost of
capturing more VA for invalidations which have to be probed against
the HW shadow PTEs.

> It isn't that expensive, there is an extra driver lock/unlock as
> part of the lookup and possibly a find_vma() and kmalloc(GFP_ATOMIC)
> for new intervals. Also, the deferred interval updates for munmap().
> Compared to the cost of updating PTEs in the device and GPU fault
> handling, this is minimal overhead.

Well, compared to ODP which does a single xa lookup with no lock to
find its interval, this looks very expensive and not parallel.

I think if there is merit in having ranges cover the vmas and track
changes then there is probably merit in having the core code provide
much of that logic, not the driver.

But it would be interesting to see some kind of analysis on the two
methods to decide if the complexity is worthwhile.

Jason

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-13 22:46 [PATCH v6 0/6] mm/hmm/test: add self tests for HMM Ralph Campbell
2020-01-13 22:46 ` [PATCH v6 1/6] mm/mmu_notifier: add mmu_interval_notifier_insert_safe() Ralph Campbell
2020-01-16 10:07   ` Christoph Hellwig
2020-01-13 22:46 ` [PATCH v6 2/6] mm/mmu_notifier: add mmu_interval_notifier_put() Ralph Campbell
2020-01-13 22:47 ` [PATCH v6 3/6] mm/notifier: add mmu_interval_notifier_update() Ralph Campbell
2020-01-13 22:47 ` [PATCH v6 4/6] mm/mmu_notifier: add mmu_interval_notifier_find() Ralph Campbell
2020-01-14 12:49   ` Jason Gunthorpe
2020-01-15 22:05     ` Ralph Campbell
2020-01-16 14:11       ` Jason Gunthorpe
2020-01-13 22:47 ` [PATCH v6 5/6] nouveau: use new mmu interval notifiers Ralph Campbell
2020-01-14 13:00   ` Jason Gunthorpe
2020-01-15 22:09     ` Ralph Campbell
2020-01-16 16:00       ` Jason Gunthorpe [this message]
2020-01-16 20:16         ` Ralph Campbell
2020-01-16 20:21           ` Jason Gunthorpe
2020-01-13 22:47 ` [PATCH v6 6/6] mm/hmm/test: add self tests for HMM Ralph Campbell

Reply instructions:

You may reply publically 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=20200116160002.GL20978@mellanox.com \
    --to=jgg@mellanox.com \
    --cc=akpm@linux-foundation.org \
    --cc=bskeggs@redhat.com \
    --cc=hch@lst.de \
    --cc=jglisse@redhat.com \
    --cc=jhubbard@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=nouveau@lists.freedesktop.org \
    --cc=rcampbell@nvidia.com \
    --cc=shuah@kernel.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

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git