Message-ID: <alpine.DEB.2.00.1506261958010.22464@mdh-linux64-2.nvidia.com>
Date:	Fri, 26 Jun 2015 20:02:03 -0700
From:	Mark Hairgrove <mhairgrove@...dia.com>
To:	Jerome Glisse <j.glisse@...il.com>
CC:	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"joro@...tes.org" <joro@...tes.org>, Mel Gorman <mgorman@...e.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Johannes Weiner <jweiner@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Dave Airlie <airlied@...hat.com>,
	Brendan Conoboy <blc@...hat.com>,
	Joe Donohue <jdonohue@...hat.com>,
	Duncan Poole <dpoole@...dia.com>,
	Sherry Cheung <SCheung@...dia.com>,
	Subhash Gutti <sgutti@...dia.com>,
	John Hubbard <jhubbard@...dia.com>,
	Lucien Dunning <ldunning@...dia.com>,
	Cameron Buschardt <cabuschardt@...dia.com>,
	Arvind Gopalakrishnan <arvindg@...dia.com>,
	Haggai Eran <haggaie@...lanox.com>,
	Shachar Raindel <raindel@...lanox.com>,
	Liran Liss <liranl@...lanox.com>,
	Roland Dreier <roland@...estorage.com>,
	Ben Sander <ben.sander@....com>,
	Greg Stoner <Greg.Stoner@....com>,
	John Bridgman <John.Bridgman@....com>,
	Michael Mantor <Michael.Mantor@....com>,
	Paul Blinzer <Paul.Blinzer@....com>,
	Laurent Morichetti <Laurent.Morichetti@....com>,
	Alexander Deucher <Alexander.Deucher@....com>,
	Oded Gabbay <Oded.Gabbay@....com>,
	Jérôme Glisse <jglisse@...hat.com>,
	Jatin Kumar <jakumar@...dia.com>
Subject: Re: [PATCH 07/36] HMM: add per mirror page table v3.



On Fri, 26 Jun 2015, Jerome Glisse wrote:

> On Thu, Jun 25, 2015 at 04:05:48PM -0700, Mark Hairgrove wrote:
> > On Thu, 21 May 2015, j.glisse@...il.com wrote:
> > > From: Jérôme Glisse <jglisse@...hat.com>
> > > [...]
> > >  
> > > +	/* update() - update device mmu following an event.
> > > +	 *
> > > +	 * @mirror: The mirror that link process address space with the device.
> > > +	 * @event: The event that triggered the update.
> > > +	 * Returns: 0 on success or error code {-EIO, -ENOMEM}.
> > > +	 *
> > > +	 * Called to update device page table for a range of address.
> > > +	 * The event type provide the nature of the update :
> > > +	 *   - Range is no longer valid (munmap).
> > > +	 *   - Range protection changes (mprotect, COW, ...).
> > > +	 *   - Range is unmapped (swap, reclaim, page migration, ...).
> > > +	 *   - Device page fault.
> > > +	 *   - ...
> > > +	 *
> > > +	 * Thought most device driver only need to use pte_mask as it reflects
> > > +	 * change that will happen to the HMM page table ie :
> > > +	 *   new_pte = old_pte & event->pte_mask;
> > 
> > Documentation request: It would be useful to break down exactly what is 
> > required from the driver for each event type here, and what extra 
> > information is provided by the type that isn't provided by the pte_mask.
> 
> Mostly event tell you if you need to free or not the device page table for
> the range, which is not something you can infer from the pte_mask reliably.
> Difference btw migration and munmap for instance, same pte_mask but range
> is still valid in the migration case it will just be backed by a new set of
> pages.

Given that event->pte_mask and event->type provide redundant information, 
are they both necessary?

With or without pte_mask, the below table would be helpful to have in the 
comments for the ->update callback:

Event type          Driver action
HMM_NONE            N/A (driver will never get this)

HMM_FORK            Same as HMM_WRITE_PROTECT

HMM_ISDIRTY         Same as HMM_WRITE_PROTECT

HMM_MIGRATE         Make device PTEs invalid and use hmm_pte_set_dirty or
                    hmm_mirror_range_dirty if applicable

HMM_MUNMAP          Same as HMM_MIGRATE, but the driver may take this as a
                    hint to free device page tables and other resources
                    associated with this range

HMM_DEVICE_RFAULT   Read hmm_ptes using hmm_pt_iter and write them on the
                    device

HMM_DEVICE_WFAULT   Same as HMM_DEVICE_RFAULT

HMM_WRITE_PROTECT   Remove write permission from device PTEs and use
                    hmm_pte_set_dirty or hmm_mirror_range_dirty if
                    applicable


> 
> 
> [...]
> > > @@ -142,6 +223,7 @@ int hmm_device_unregister(struct hmm_device *device);
> > >   * @kref: Reference counter (private to HMM do not use).
> > >   * @dlist: List of all hmm_mirror for same device.
> > >   * @mlist: List of all hmm_mirror for same process.
> > > + * @pt: Mirror page table.
> > >   *
> > >   * Each device that want to mirror an address space must register one of this
> > >   * struct for each of the address space it wants to mirror. Same device can
> > > @@ -154,6 +236,7 @@ struct hmm_mirror {
> > >  	struct kref		kref;
> > >  	struct list_head	dlist;
> > >  	struct hlist_node	mlist;
> > > +	struct hmm_pt		pt;
> > 
> > Documentation request: Why does each mirror have its own separate set of 
> > page tables rather than the hmm keeping one set for all devices? This is 
> > so different devices can have different permissions for the same address 
> > range, correct?
> 
> Several reasons, first and mostly dma mapping, while i have plan to allow
> to share dma mapping directory btw devices this require work in the dma
> layer first. Second reasons is, like you point out, different permissions,
> like one device requesting atomic access ie the device will be the only
> one with write permission and HMM need somewhere to store that information
> per device per address. It also helps to avoid calling device driver on a
> range that one device does not mirror.

Sure, that makes sense. Can you put this in the documentation somewhere, 
perhaps in the header comments for struct hmm_mirror?

Thanks!

Powered by blists - more mailing lists