All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Kuehling, Felix" <felix.kuehling@amd.com>
To: William Palacek <William.Palacek@amd.com>, amd-gfx@lists.freedesktop.org
Cc: Harish.Kasiviswanathan@amd.com, kent.russell@amd.com
Subject: Re: [PATCH] drm/amdkfd: skip migration when the fault window is already in VRAM
Date: Tue, 1 Sep 2026 11:56:26 -0400	[thread overview]
Message-ID: <813dc7e9-d7f3-4d8d-a641-7873e4c9a32d@amd.com> (raw)
In-Reply-To: <20260901124516.357959-1-William.Palacek@amd.com>

[-- Attachment #1: Type: text/plain, Size: 7000 bytes --]


On 2026-09-01 08:45, William Palacek wrote:
> svm_migrate_vma_to_vram() is called for windows that are already fully
> resident in the target GPU's VRAM. migrate_vma_setup() raises an MMU
> notifier invalidate before it can discover there is nothing to collect,
> and with retry faults enabled svm_range_cpu_invalidate_pagetables()
> special cases only MMU_NOTIFY_UNMAP, so MMU_NOTIFY_MIGRATE is handled by
> its default arm, reaching svm_range_evict() and then
> svm_range_unmap_from_gpus().
>
> The attempt therefore drops the window's GPU mapping and then returns
> having migrated nothing, so the next fault on that window is escalated
> into another migration instead of being satisfied by mapping alone.
>
> Measured on MI300X (gfx942, SPX/NPS1, xnack+) under an HMM
> oversubscription workload, 96 to 97% of to-VRAM migration attempts
> collected zero pages, and the driver unmapped a window roughly 35 times
> for every migration that moved data.
>
> Test whether every page of the faulting window is already resident in
> this node's VRAM and return early if it is. prange->actual_loc alone is
> not sufficient: since commit a546a2768440 ("drm/amdkfd: Use partial
> migrations/mapping for GPU/CPU page faults in SVM") migration is per
> window, so a range can report actual_loc == best_loc while part of it has
> been evicted back to system memory. 3.3% of attempts were in exactly that
> state and did have pages to move, so the per-page test is required rather
> than the scalar alone. Instrumented over 100000 attempts on an earlier
> run against a different driver build, the per-page test never skipped one
> that had pages to move.
>
> With this applied the fault driven phase of that workload falls from 270
> to 348 s down to 45 to 48 s, and page_in at the end of that phase is 46.2
> to 48.0M pages against 44.1 to 45.7M unpatched, so the same or more data
> is moved. Unmaps per productive migration fall from about 35 to about 2.
> Retry fault counts do not fall in any arm, so this is not a refault loop;
> what changes is how often a fault escalates into a migration.
>
> The scan is bounded by the migration window, one granule on the fault
> path and the whole range on the prefetch path. That is the same order as
> the migrate_vma_setup() walk it avoids, and it returns at the first page
> that is not resident, so the worst case is one extra pass over a range
> that then migrates normally.
>
> Signed-off-by: William Palacek<William.Palacek@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 65 ++++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> index c3411dacdf55..40fc21d18587 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> @@ -384,6 +384,52 @@ svm_migrate_copy_to_vram(struct kfd_node *node, struct svm_range *prange,
>   	return r;
>   }
>   
> +/* Is every page of [start, end) already resident in the VRAM of @node?
> + *
> + * prange->actual_loc is a per-range scalar and is not sufficient on its own.
> + * Since commit a546a2768440 ("drm/amdkfd: Use partial migrations/mapping for
> + * GPU/CPU page faults in SVM") migration is per window, so a range can report
> + * actual_loc == best_loc while parts of it have been evicted back to system
> + * memory. On a gfx942 oversubscription workload 3.3% of migration attempts
> + * were on windows in exactly that state and did have pages to move, so the
> + * per-page test below is needed rather than the scalar alone.
> + *
> + * SVM_RANGE_VRAM_DOMAIN records "some device VRAM", not which device, which
> + * is why the actual_loc comparison is kept as well.
> + *
> + * Called with prange->migrate_mutex held.

I'm OK with a long explanation in the commit description. But for the 
code comment I'd prefer to keep it more concise:

Check if a range of addresses is already resident in VRAM to avoid 
unnecessary migrations that would trigger MMU notifiers causing the 
range to be unmapped and faulted again unnecessarily.


> + */
> +static bool
> +svm_range_window_resident(struct kfd_node *node, struct svm_range *prange,
> +			  u64 start, u64 end)
> +{
> +	struct kfd_process *p = container_of(prange->svms, struct kfd_process, svms);
> +	unsigned long i, offset, wpages;
> +	dma_addr_t *addr;
> +	uint32_t gpuid;
> +	int32_t gpuidx;
> +
> +	if (kfd_process_gpuid_from_node(p, node, &gpuid, &gpuidx))
> +		return false;
> +	if (prange->actual_loc != gpuid)
> +		return false;
> +
> +	addr = prange->dma_addr[gpuidx];
> +	if (!addr)
> +		return false;
> +
> +	offset = (start >> PAGE_SHIFT) - prange->start;
> +	wpages = (end - start) >> PAGE_SHIFT;
> +	if (offset + wpages > prange->npages)
> +		return false;

I believe this case shouldn't happen in practice. If it did, then 
returning false here would be conservatively correct but may result in 
unnecessary migration attempts.

If you wanted to be really sure, you'd also need to protect against a 
negative offset (or integer underflow). I think a more robust approach 
would be to adjust the bounds properly to fit within prange:

	start = max(start >> PAGE_SHIFT, prange->start);
	end = min(end >> PAGE_SHIFT, prange->end);
	offset = start - prange->start;
	npages = end - start;


> +
> +	for (i = offset; i < offset + wpages; i++)
> +		if (!(addr[i] & SVM_RANGE_VRAM_DOMAIN))
> +			return false;
> +
> +	return true;
> +}
> +
>   static long
>   svm_migrate_vma_to_vram(struct kfd_node *node, struct svm_range *prange,
>   			struct vm_area_struct *vma, u64 start,
> @@ -401,6 +447,25 @@ svm_migrate_vma_to_vram(struct kfd_node *node, struct svm_range *prange,
>   	void *buf;
>   	int r = -ENOMEM;
>   
> +	/* Every page of this window is already in this node's VRAM, so
> +	 * migrate_vma_setup() below can collect nothing. It issues the
> +	 * MMU-notifier invalidate before discovering that, and with retry
> +	 * faults enabled that lands in svm_range_cpu_invalidate_pagetables(),
> +	 * which special cases only MMU_NOTIFY_UNMAP, so MMU_NOTIFY_MIGRATE is
> +	 * handled by its default arm, reaching svm_range_evict() and
> +	 * svm_range_unmap_from_gpus(). The attempt therefore tears this
> +	 * window's GPU mapping down and then finds nothing to move, so the
> +	 * next fault on it is escalated back into migration instead of being
> +	 * satisfied by mapping alone.
> +	 *
> +	 * On a gfx942 oversubscription workload such attempts were 96 to 97%
> +	 * of all to-VRAM migrations, and the driver unmapped a window roughly
> +	 * 35 times per migration that moved data, against about 2 with this
> +	 * guard.
> +	 */

Again, I'd prefer a shorter comment here:

Avoid unnecessary migrations and MMU notifiers if all the data is 
already resident in VRAM.

Regards,
   Felix


> +	if (svm_range_window_resident(node, prange, start, end))
> +		return 0;
> +
>   	memset(&migrate, 0, sizeof(migrate));
>   	migrate.vma = vma;
>   	migrate.start = start;

[-- Attachment #2: Type: text/html, Size: 7950 bytes --]

      reply	other threads:[~2026-09-01 15:56 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-01 12:45 [PATCH] drm/amdkfd: skip migration when the fault window is already in VRAM William Palacek
2026-09-01 15:56 ` Kuehling, Felix [this message]

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=813dc7e9-d7f3-4d8d-a641-7873e4c9a32d@amd.com \
    --to=felix.kuehling@amd.com \
    --cc=Harish.Kasiviswanathan@amd.com \
    --cc=William.Palacek@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=kent.russell@amd.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.