Re: [PATCH hmm 15/15] mm/hmm: remove hmm_mirror and related
On Mon, Oct 21, 2019 at 02:38:24PM -0400, Jerome Glisse wrote: > On Tue, Oct 15, 2019 at 03:12:42PM -0300, Jason Gunthorpe wrote: > > From: Jason Gunthorpe > > > > The only two users of this are now converted to use mmu_range_notifier, > > delete all the code and update hmm.rst. > > I guess i should point out that the reasons for hmm_mirror and hmm > was for: > 1) Maybe define a common API for userspace to provide memory >placement hints (NUMA for GPU) Do you think this needs special code in the notifiers? > 2) multi-devices sharing same mirror page table Oh neat, but I think this just means the GPU driver has to register a single notifier for multiple GPUs?? > But support for multi-GPU in nouveau is way behind and i guess such > optimization will have to re-materialize what is necessary once that > happens. Sure, it will be easier to understand what is needed with a bit of code! > Note this patch should also update kernel/fork.c and the mm_struct > definition AFAICT. With those changes you can add my: Can you please elaborate what updates you mean? I'm not sure. Maybe I already got the things you are thinking of with the get/put changes? Thanks, Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH hmm 15/15] mm/hmm: remove hmm_mirror and related
On Mon, Oct 21, 2019 at 06:57:42PM +, Jason Gunthorpe wrote: > On Mon, Oct 21, 2019 at 02:38:24PM -0400, Jerome Glisse wrote: > > On Tue, Oct 15, 2019 at 03:12:42PM -0300, Jason Gunthorpe wrote: > > > From: Jason Gunthorpe > > > > > > The only two users of this are now converted to use mmu_range_notifier, > > > delete all the code and update hmm.rst. > > > > I guess i should point out that the reasons for hmm_mirror and hmm > > was for: > > 1) Maybe define a common API for userspace to provide memory > >placement hints (NUMA for GPU) > > Do you think this needs special code in the notifiers? Just need a place where to hang userspace policy hint the hmm_range was the prime suspect. I need to revisit this once the nouveau user space is in better shape. > > > 2) multi-devices sharing same mirror page table > > Oh neat, but I think this just means the GPU driver has to register a > single notifier for multiple GPUs?? Yes that was the idea a single notifier with share page table, but at this time this is non existent code so no need to hinder change just for the sake of it. > > > But support for multi-GPU in nouveau is way behind and i guess such > > optimization will have to re-materialize what is necessary once that > > happens. > > Sure, it will be easier to understand what is needed with a bit of > code! > > > Note this patch should also update kernel/fork.c and the mm_struct > > definition AFAICT. With those changes you can add my: > > Can you please elaborate what updates you mean? I'm not sure. > > Maybe I already got the things you are thinking of with the get/put > changes? Oh i forgot this was already taken care of by this. So yes all is fine: Reviewed-by: Jérôme Glisse ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH hmm 15/15] mm/hmm: remove hmm_mirror and related
On Tue, Oct 15, 2019 at 03:12:42PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe
>
> The only two users of this are now converted to use mmu_range_notifier,
> delete all the code and update hmm.rst.
I guess i should point out that the reasons for hmm_mirror and hmm
was for:
1) Maybe define a common API for userspace to provide memory
placement hints (NUMA for GPU)
2) multi-devices sharing same mirror page table
But support for multi-GPU in nouveau is way behind and i guess such
optimization will have to re-materialize what is necessary once that
happens.
Note this patch should also update kernel/fork.c and the mm_struct
definition AFAICT. With those changes you can add my:
Reviewed-by: Jérôme Glisse
>
> Signed-off-by: Jason Gunthorpe
> ---
> Documentation/vm/hmm.rst | 105 ---
> include/linux/hmm.h | 183 +
> mm/Kconfig | 1 -
> mm/hmm.c | 284 +--
> 4 files changed, 33 insertions(+), 540 deletions(-)
>
> diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
> index 0a5960beccf76d..a247643035c4e2 100644
> --- a/Documentation/vm/hmm.rst
> +++ b/Documentation/vm/hmm.rst
> @@ -147,49 +147,16 @@ Address space mirroring implementation and API
> Address space mirroring's main objective is to allow duplication of a range
> of
> CPU page table into a device page table; HMM helps keep both synchronized. A
> device driver that wants to mirror a process address space must start with
> the
> -registration of an hmm_mirror struct::
> -
> - int hmm_mirror_register(struct hmm_mirror *mirror,
> - struct mm_struct *mm);
> -
> -The mirror struct has a set of callbacks that are used
> -to propagate CPU page tables::
> -
> - struct hmm_mirror_ops {
> - /* release() - release hmm_mirror
> - *
> - * @mirror: pointer to struct hmm_mirror
> - *
> - * This is called when the mm_struct is being released. The callback
> - * must ensure that all access to any pages obtained from this mirror
> - * is halted before the callback returns. All future access should
> - * fault.
> - */
> - void (*release)(struct hmm_mirror *mirror);
> -
> - /* sync_cpu_device_pagetables() - synchronize page tables
> - *
> - * @mirror: pointer to struct hmm_mirror
> - * @update: update information (see struct mmu_notifier_range)
> - * Return: -EAGAIN if update.blockable false and callback need to
> - * block, 0 otherwise.
> - *
> - * This callback ultimately originates from mmu_notifiers when the CPU
> - * page table is updated. The device driver must update its page table
> - * in response to this callback. The update argument tells what action
> - * to perform.
> - *
> - * The device driver must not return from this callback until the device
> - * page tables are completely updated (TLBs flushed, etc); this is a
> - * synchronous call.
> - */
> - int (*sync_cpu_device_pagetables)(struct hmm_mirror *mirror,
> - const struct hmm_update *update);
> - };
> -
> -The device driver must perform the update action to the range (mark range
> -read only, or fully unmap, etc.). The device must complete the update before
> -the driver callback returns.
> +registration of a mmu_range_notifier::
> +
> + mrn->ops = _ops;
> + int mmu_range_notifier_insert(struct mmu_range_notifier *mrn,
> + unsigned long start, unsigned long length,
> + struct mm_struct *mm);
> +
> +During the driver_ops->invalidate() callback the device driver must perform
> +the update action to the range (mark range read only, or fully unmap,
> +etc.). The device must complete the update before the driver callback
> returns.
>
> When the device driver wants to populate a range of virtual addresses, it can
> use::
> @@ -216,70 +183,46 @@ The usage pattern is::
>struct hmm_range range;
>...
>
> + range.notifier =
>range.start = ...;
>range.end = ...;
>range.pfns = ...;
>range.flags = ...;
>range.values = ...;
>range.pfn_shift = ...;
> - hmm_range_register(, mirror);
>
> - /*
> - * Just wait for range to be valid, safe to ignore return value as we
> - * will use the return value of hmm_range_fault() below under the
> - * mmap_sem to ascertain the validity of the range.
> - */
> - hmm_range_wait_until_valid(, TIMEOUT_IN_MSEC);
> + if (!mmget_not_zero(mrn->notifier.mm))
> + return -EFAULT;
>
> again:
> + range.notifier_seq = mmu_range_read_begin();
>down_read(>mmap_sem);
>ret = hmm_range_fault(, HMM_RANGE_SNAPSHOT);
>if (ret) {
>up_read(>mmap_sem);
> - if (ret == -EBUSY) {
> -/*
> - * No need to
[PATCH hmm 15/15] mm/hmm: remove hmm_mirror and related
From: Jason Gunthorpe
The only two users of this are now converted to use mmu_range_notifier,
delete all the code and update hmm.rst.
Signed-off-by: Jason Gunthorpe
---
Documentation/vm/hmm.rst | 105 ---
include/linux/hmm.h | 183 +
mm/Kconfig | 1 -
mm/hmm.c | 284 +--
4 files changed, 33 insertions(+), 540 deletions(-)
diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
index 0a5960beccf76d..a247643035c4e2 100644
--- a/Documentation/vm/hmm.rst
+++ b/Documentation/vm/hmm.rst
@@ -147,49 +147,16 @@ Address space mirroring implementation and API
Address space mirroring's main objective is to allow duplication of a range of
CPU page table into a device page table; HMM helps keep both synchronized. A
device driver that wants to mirror a process address space must start with the
-registration of an hmm_mirror struct::
-
- int hmm_mirror_register(struct hmm_mirror *mirror,
- struct mm_struct *mm);
-
-The mirror struct has a set of callbacks that are used
-to propagate CPU page tables::
-
- struct hmm_mirror_ops {
- /* release() - release hmm_mirror
- *
- * @mirror: pointer to struct hmm_mirror
- *
- * This is called when the mm_struct is being released. The callback
- * must ensure that all access to any pages obtained from this mirror
- * is halted before the callback returns. All future access should
- * fault.
- */
- void (*release)(struct hmm_mirror *mirror);
-
- /* sync_cpu_device_pagetables() - synchronize page tables
- *
- * @mirror: pointer to struct hmm_mirror
- * @update: update information (see struct mmu_notifier_range)
- * Return: -EAGAIN if update.blockable false and callback need to
- * block, 0 otherwise.
- *
- * This callback ultimately originates from mmu_notifiers when the CPU
- * page table is updated. The device driver must update its page table
- * in response to this callback. The update argument tells what action
- * to perform.
- *
- * The device driver must not return from this callback until the device
- * page tables are completely updated (TLBs flushed, etc); this is a
- * synchronous call.
- */
- int (*sync_cpu_device_pagetables)(struct hmm_mirror *mirror,
- const struct hmm_update *update);
- };
-
-The device driver must perform the update action to the range (mark range
-read only, or fully unmap, etc.). The device must complete the update before
-the driver callback returns.
+registration of a mmu_range_notifier::
+
+ mrn->ops = _ops;
+ int mmu_range_notifier_insert(struct mmu_range_notifier *mrn,
+ unsigned long start, unsigned long length,
+ struct mm_struct *mm);
+
+During the driver_ops->invalidate() callback the device driver must perform
+the update action to the range (mark range read only, or fully unmap,
+etc.). The device must complete the update before the driver callback returns.
When the device driver wants to populate a range of virtual addresses, it can
use::
@@ -216,70 +183,46 @@ The usage pattern is::
struct hmm_range range;
...
+ range.notifier =
range.start = ...;
range.end = ...;
range.pfns = ...;
range.flags = ...;
range.values = ...;
range.pfn_shift = ...;
- hmm_range_register(, mirror);
- /*
- * Just wait for range to be valid, safe to ignore return value as we
- * will use the return value of hmm_range_fault() below under the
- * mmap_sem to ascertain the validity of the range.
- */
- hmm_range_wait_until_valid(, TIMEOUT_IN_MSEC);
+ if (!mmget_not_zero(mrn->notifier.mm))
+ return -EFAULT;
again:
+ range.notifier_seq = mmu_range_read_begin();
down_read(>mmap_sem);
ret = hmm_range_fault(, HMM_RANGE_SNAPSHOT);
if (ret) {
up_read(>mmap_sem);
- if (ret == -EBUSY) {
-/*
- * No need to check hmm_range_wait_until_valid() return value
- * on retry we will get proper error with hmm_range_fault()
- */
-hmm_range_wait_until_valid(, TIMEOUT_IN_MSEC);
-goto again;
- }
- hmm_range_unregister();
+ if (ret == -EBUSY)
+ goto again;
return ret;
}
+ up_read(>mmap_sem);
+
take_lock(driver->update);
- if (!hmm_range_valid()) {
+ if (mmu_range_read_retry(, range.notifier_seq) {
release_lock(driver->update);
- up_read(>mmap_sem);
goto again;
}
- // Use pfns array content to update device page table
+ /* Use pfns array content to update device page table,
+ * under the update lock */
- hmm_range_unregister();
