Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1172511 > unrolled thread

Re: [PATCH 07/36] HMM: add per mirror page table v3.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 07/36] HMM: add per mirror page table v3. Mark Hairgrove <mhairgrove@nvidia.com> - 2015-06-26 01:10 +0200
    Re: [PATCH 07/36] HMM: add per mirror page table v3. Jerome Glisse <j.glisse@gmail.com> - 2015-06-29 17:00 +0200

#1172511 — Re: [PATCH 07/36] HMM: add per mirror page table v3.

[Multipart message — attachments visible in raw view] — view raw

On Thu, 21 May 2015, j.glisse@gmail.com wrote:

> From: Jérôme Glisse <jglisse@redhat.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.

> +	 *
> +	 * Device driver must not update the HMM mirror page table (except the
> +	 * dirty bit see below). Core HMM will update HMM page table after the
> +	 * update is done.
> +	 *
> +	 * Note that device must be cache coherent with system memory (snooping
> +	 * in case of PCIE devices) so there should be no need for device to
> +	 * flush anything.
> +	 *
> +	 * When write protection is turned on device driver must make sure the
> +	 * hardware will no longer be able to write to the page otherwise file
> +	 * system corruption may occur.
> +	 *
> +	 * Device must properly set the dirty bit using hmm_pte_set_bit() on
> +	 * each page entry for memory that was written by the device. If device
> +	 * can not properly account for write access then the dirty bit must be
> +	 * set unconditionaly so that proper write back of file backed page can
> +	 * happen.
> +	 *
> +	 * Device driver must not fail lightly, any failure result in device
> +	 * process being kill.
> +	 *
> +	 * Return 0 on success, error value otherwise :
> +	 * -ENOMEM Not enough memory for performing the operation.
> +	 * -EIO    Some input/output error with the device.
> +	 *
> +	 * All other return value trigger warning and are transformed to -EIO.
> +	 */
> +	int (*update)(struct hmm_mirror *mirror,const struct hmm_event *event);
>  };
>  
>  
> @@ -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?

>  };
>  
> [...]
> +
> +static inline int hmm_event_init(struct hmm_event *event,
> +				 struct hmm *hmm,
> +				 unsigned long start,
> +				 unsigned long end,
> +				 enum hmm_etype etype)
> +{
> +	event->start = start & PAGE_MASK;
> +	event->end = min(end, hmm->vm_end);

start is rounded down to a page boundary. Should end be rounded also?


> [...]
> +
> +static void hmm_mirror_update_pt(struct hmm_mirror *mirror,
> +				 struct hmm_event *event)
> +{
> +	unsigned long addr;
> +	struct hmm_pt_iter iter;
> +
> +	hmm_pt_iter_init(&iter);
> +	for (addr = event->start; addr != event->end;) {
> +		unsigned long end, next;
> +		dma_addr_t *hmm_pte;
> +
> +		hmm_pte = hmm_pt_iter_update(&iter, &mirror->pt, addr);
> +		if (!hmm_pte) {
> +			addr = hmm_pt_iter_next(&iter, &mirror->pt,
> +						addr, event->end);
> +			continue;
> +		}
> +		end = hmm_pt_level_next(&mirror->pt, addr, event->end,
> +					 mirror->pt.llevel - 1);
> +		/*
> +		 * The directory lock protect against concurrent clearing of
> +		 * page table bit flags. Exceptions being the dirty bit and
> +		 * the device driver private flags.
> +		 */
> +		hmm_pt_iter_directory_lock(&iter, &mirror->pt);
> +		do {
> +			next = hmm_pt_level_next(&mirror->pt, addr, end,
> +						 mirror->pt.llevel);
> +			if (!hmm_pte_test_valid_pfn(hmm_pte))
> +				continue;
> +			if (hmm_pte_test_and_clear_dirty(hmm_pte) &&
> +			    hmm_pte_test_write(hmm_pte)) {

If the pte is dirty, why bother checking that it's writable?

Could there be a legitimate case in which the page was dirtied in the 
past, but was made read-only later for some reason? In that case the page 
would still need to be be dirtied correctly even though the hmm_pte isn't 
currently writable.

Or is this check trying to protect against a driver setting the dirty bit 
without the write bit being set? If that happens, that's a driver bug, 
right?

> +				struct page *page;
> +
> +				page = pfn_to_page(hmm_pte_pfn(*hmm_pte));
> +				set_page_dirty(page);
> +			}
> +			*hmm_pte &= event->pte_mask;
> +			if (hmm_pte_test_valid_pfn(hmm_pte))
> +				continue;
> +			hmm_pt_iter_directory_unref(&iter, mirror->pt.llevel);
> +		} while (addr = next, hmm_pte++, addr != end);
> +		hmm_pt_iter_directory_unlock(&iter, &mirror->pt);
> +	}
> +	hmm_pt_iter_fini(&iter, &mirror->pt);
> +}

[toc] | [next] | [standalone]

#1173853

On Fri, Jun 26, 2015 at 08:02:03PM -0700, Mark Hairgrove wrote:
> 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@gmail.com wrote:
> > > > From: Jérôme Glisse <jglisse@redhat.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?

Like said, you can not infer event->type from pte_mask but you can infer
pte_mask from event->type. The idea is behind providing pte_mask is that
simple driver can just use that with the iter walk and simply mask the HMM
page table entry they read ((*ptep) & pte_mask) to repopulate the device
page table.

So yes pte_mask is redundant but i think it will be useful for a range of
device driver.

Cheers,
Jérôme
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web