Re: [PATCH V2 2/3] drm/xe/vram: don't let firmware-confirmed permanent bad pages decay back in

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/xe/vram: queued pages not promoted to HARD on repeat
  fault
- [Medium] drm/xe/vram: decay_gc_work not scheduled in
  xe_ttm_vram_retry_queued_pages()
- [Low] drm/xe/vram: missing @permanent kernel-doc on
  xe_ttm_vram_handle_addr_fault()
--

commit 49821b1409082008ca85432821b443bc32f84c8f
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>

drm/xe/vram: don't let firmware-confirmed permanent bad pages decay back in

This commit threads a permanent flag through xe_ttm_vram_handle_addr_fault()
to xe_ttm_vram_reserve_page_at_addr() so pages loaded from firmware's
non-volatile offline list are marked XE_PAGE_OFFLINE_HARD and excluded from
soft-offline decay reinstatement.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index ed0ca5f2b1449..07360ab7b0722 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -837,7 +837,8 @@ static struct xe_bo *xe_ttm_vram_block_owner_get(struct xe_device *xe,
>  }
>  
>  static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
> -					    struct xe_ttm_vram_mgr *vram_mgr, struct gpu_buddy *mm)
> +					    struct xe_ttm_vram_mgr *vram_mgr, struct gpu_buddy *mm,
> +					    bool permanent)
>  {
>  	struct xe_ttm_vram_offline_resource *nentry;
>  	struct xe_bo *pbo_to_put = NULL;
> @@ -859,7 +860,11 @@ static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
>  		INIT_LIST_HEAD(&nentry->blocks);
>  		nentry->status = XE_PAGE_RESERVE_PENDING;
>  		nentry->addr = addr;
> -		nentry->mode = XE_PAGE_OFFLINE_SOFT;
> +		/*
> +		 * Pages reloaded from firmware's non-volatile permanent list are
> +		 * already confirmed bad by a prior boot; never let them decay back in.
> +		 */
> +		nentry->mode = permanent ? XE_PAGE_OFFLINE_HARD : XE_PAGE_OFFLINE_SOFT;

[Severity: Medium]
Does a repeat fault on a page that is still on vram_mgr->queued_pages leave
pos->mode as XE_PAGE_OFFLINE_SOFT even though firmware permanently retires
the page?

When a non-permanent fault hits an active BO, nentry is queued on
vram_mgr->queued_pages as XE_PAGE_OFFLINE_SOFT until the BO's resource is
freed.  If a second fault arrives for the same address before
xe_ttm_vram_mgr_del() runs, xe_ttm_vram_page_already_processed() matches on
mgr->queued_pages without promoting pos->mode:

drivers/gpu/drm/xe/xe_ttm_vram_mgr.c:xe_ttm_vram_page_already_processed() {
	list_for_each_entry(pos, &mgr->offlined_pages, offlined_link) {
		if (pos->addr == addr) {
			if (pos->mode == XE_PAGE_OFFLINE_SOFT)
				pos->mode = XE_PAGE_OFFLINE_HARD;
			pos->last_seen = jiffies;
			return true;
		}
	}

	list_for_each_entry(pos, &mgr->queued_pages, queued_link) {
		if (pos->addr == addr)
			return true;
	}
}

handle_page_offline() receives -EEXIST and sends XE_RAS_PAGE_ACTION_OFFLINE
to permanently retire the page in firmware, but when
xe_ttm_vram_retry_queued_pages() later moves the entry to
mgr->offlined_pages, it remains XE_PAGE_OFFLINE_SOFT and will be reinstated
by xe_ttm_vram_decay_gc_worker().

>  		nentry->last_seen = jiffies;
>  
>  		if (block) {
> @@ -900,7 +905,7 @@ static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
>  			}
>  			++vram_mgr->n_offlined_pages;
>  			list_add(&nentry->offlined_link, &vram_mgr->offlined_pages);
> -			if (vram_mgr->decay_interval_jiffies &&
> +			if (!permanent && vram_mgr->decay_interval_jiffies &&
>  			    !delayed_work_pending(&vram_mgr->decay_gc_work)) {
>  				schedule_delayed_work(&vram_mgr->decay_gc_work,
>  						      xe_ttm_vram_decay_gc_period(vram_mgr));

[Severity: Medium]
Should xe_ttm_vram_retry_queued_pages() also schedule vram_mgr->decay_gc_work
when moving a soft-offlined entry onto mgr->offlined_pages?

When a runtime fault hits an allocated page (block != NULL), nentry is added
to vram_mgr->queued_pages without starting decay_gc_work, and
xe_ttm_vram_retry_queued_pages() later moves it to mgr->offlined_pages
without scheduling the worker either:

drivers/gpu/drm/xe/xe_ttm_vram_mgr.c:xe_ttm_vram_retry_queued_pages() {
	list_for_each_entry_safe(pos, n, &mgr->queued_pages, queued_link) {
		if (xe_ttm_vram_buddy_alloc(mgr, pos->addr, pos->addr + PAGE_SIZE,
					    PAGE_SIZE, PAGE_SIZE, &pos->blocks,
					    GPU_BUDDY_RANGE_ALLOCATION, NULL,
					    &pos->used_visible_size)) {
			pos->status = XE_PAGE_RESERVE_FAIL;
			continue;
		}
		--mgr->n_queued_pages;
		list_del(&pos->queued_link);
		++mgr->n_offlined_pages;
		list_add(&pos->offlined_link, &mgr->offlined_pages);
	}
}

Because probe-time permanent pages no longer start decay_gc_work, soft-offline
faults on allocated BOs will never start the decay timer unless an unrelated
fault happens to hit an unallocated page.

> @@ -986,7 +991,7 @@ static struct xe_vram_region *xe_ttm_vram_addr_to_region(struct xe_device *xe, u
>   * pending if the blocks backing it could not be freed immediately; the
>   * reservation is then completed from xe_ttm_vram_mgr_del().
>   */
> -int xe_ttm_vram_handle_addr_fault(struct xe_device *xe, u64 addr)
> +int xe_ttm_vram_handle_addr_fault(struct xe_device *xe, u64 addr, bool permanent)

[Severity: Low]
The kernel-doc comment above xe_ttm_vram_handle_addr_fault() documents @xe
and @addr, but does not include the new @permanent parameter, which triggers
a kernel-doc warning under W=1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112124329.2960272-5-tejas.upadhyay@intel.com?part=2