Re: [PATCH] drm/xe/vram: add decay-based reinstatement of soft-offlined VRAM pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/xe: permanent bad pages from get_offline_list() marked SOFT and reinstated
- [High] drm/xe: queued pages not promoted to HARD on repeat fault
- [High] drm/xe: decay_gc_work not armed in xe_ttm_vram_retry_queued_pages()
- [Low] drm/xe: missing stub for xe_configfs_get_bad_page_decay_secs()
- [Low] drm/xe: xe_ttm_vram_decay_gc_worker() reschedules itself unconditionally
- [Low] drm/xe: bad_page_decay_secs omitted from dump_custom_dev_config()
--

commit 46cc37a480016d26168ac9267631e7a2c7b48ce8
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>

drm/xe/vram: add decay-based reinstatement of soft-offlined VRAM pages

This commit introduces a per-VRAM-manager delayed work timer to reinstate
soft-offlined VRAM pages after a configurable decay interval (defaulting to
24 hours via configfs bad_page_decay_secs) if no further faults occur at the
same address. It also adds soft/hard offline mode tracking to promote a page
from soft to permanent hard retirement upon a second fault.

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 2ddd707a6dbd3..110459b8980f4 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
> @@ -305,6 +305,7 @@ struct xe_config_group_device {
>  		bool enable_psmi;
>  		bool enable_multi_queue;
>  		bool disable_vram_page_offline;
> +		u32 bad_page_decay_secs;
>  		struct {
>  			unsigned int max_vfs;
>  			bool admin_only_pf;
> @@ -327,6 +328,7 @@ static const struct xe_config_device device_defaults = {
>  	.enable_psmi = false,
>  	.enable_multi_queue = true,
>  	.disable_vram_page_offline = false,
> +	.bad_page_decay_secs = 24 * 60 * 60,
>  	.sriov = {
>  		.max_vfs = XE_DEFAULT_MAX_VFS,
>  		.admin_only_pf = XE_DEFAULT_ADMIN_ONLY_PF,

[Severity: Low]
When bad_page_decay_secs is modified via configfs, xe_configfs_check_device()
detects the change in struct xe_config_device via memcmp() and invokes
dump_custom_dev_config(), which currently only prints:

drivers/gpu/drm/xe/xe_configfs.c:dump_custom_dev_config() {
	...
	PRI_CUSTOM_ATTR("%d", enable_psmi);
	PRI_CUSTOM_ATTR("%d", disable_vram_page_offline);
	PRI_CUSTOM_ATTR("%d", survivability_mode);
	...
}

Should dump_custom_dev_config() also include PRI_CUSTOM_ATTR("%u",
bad_page_decay_secs) so that non-default decay settings are logged alongside
the other custom attributes?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> index 327b648944ffa..406b722247020 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.h
> +++ b/drivers/gpu/drm/xe/xe_configfs.h
> @@ -26,6 +26,7 @@ bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev);
>  bool xe_configfs_get_enable_multi_queue(struct pci_dev *pdev);
>  u32 xe_configfs_get_migrate_ulls_period_ms(struct pci_dev *pdev);
>  bool xe_configfs_get_disable_vram_page_offline(struct pci_dev *pdev);
> +u32 xe_configfs_get_bad_page_decay_secs(struct pci_dev *pdev);
>  u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev,
>  				       enum xe_engine_class class,
>  				       const u32 **cs);

[Severity: Low]
Will building with CONFIG_DRM_XE enabled and !IS_REACHABLE(CONFIG_CONFIGFS_FS)
fail in __xe_ttm_vram_mgr_init()?

In drivers/gpu/drm/xe/xe_configfs.h, xe_configfs_get_bad_page_decay_secs() is
declared inside the #if IS_REACHABLE(CONFIG_CONFIGFS_FS) block, but no static
inline fallback stub was added to the corresponding #else block:

drivers/gpu/drm/xe/xe_configfs.h:
#else
...
static inline u32 xe_configfs_get_migrate_ulls_period_ms(struct pci_dev *pdev) { return 5; }
static inline bool xe_configfs_get_disable_vram_page_offline(struct pci_dev *pdev) { return false; }
static inline u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev,

Meanwhile, __xe_ttm_vram_mgr_init() in drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
calls xe_configfs_get_bad_page_decay_secs() unconditionally, and
XE_BAD_PAGE_DECAY_SECS_DEFAULT in drivers/gpu/drm/xe/xe_ttm_vram_mgr_types.h
is left unused. Should a static inline stub returning
XE_BAD_PAGE_DECAY_SECS_DEFAULT (and using that macro in device_defaults) be
added when configfs is not reachable?

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 36d47de8b11ba..ed0ca5f2b1449 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -355,6 +355,51 @@ static const struct ttm_resource_manager_func xe_ttm_vram_mgr_func = {
>  	.debug	= xe_ttm_vram_mgr_debug
>  };
>  
> +static unsigned long xe_ttm_vram_decay_gc_period(struct xe_ttm_vram_mgr *mgr)
> +{
> +	unsigned long period = mgr->decay_interval_jiffies / 8;
> +
> +	if (period < msecs_to_jiffies(60 * 1000))
> +		period = msecs_to_jiffies(60 * 1000);
> +	return period;
> +}
> +
> +static void xe_ttm_vram_decay_gc_worker(struct work_struct *work)
> +{
> +	struct xe_ttm_vram_mgr *mgr = container_of(to_delayed_work(work),
> +						   struct xe_ttm_vram_mgr,
> +						   decay_gc_work);
> +	struct xe_ttm_vram_offline_resource *pos, *n;
> +	struct xe_device *xe = ttm_to_xe_device(mgr->manager.bdev);
> +	unsigned long now = jiffies;
> +	unsigned long decay;
> +
> +	mutex_lock(&mgr->lock);
> +	decay = mgr->decay_interval_jiffies;
> +	if (!decay)
> +		goto reschedule;
> +
> +	list_for_each_entry_safe(pos, n, &mgr->offlined_pages, offlined_link) {
> +		if (pos->mode != XE_PAGE_OFFLINE_SOFT)
> +			continue;
> +		if (!time_after(now, pos->last_seen + decay))
> +			continue;
> +
> +		list_del(&pos->offlined_link);
> +		xe_ttm_vram_buddy_free(mgr, &pos->blocks, pos->used_visible_size);
> +		--mgr->n_offlined_pages;
> +		drm_info(&xe->drm,
> +			 "decay: reinstated soft-offlined page 0x%llx\n",
> +			 pos->addr);
> +		kfree(pos);
> +	}
> +
> +reschedule:
> +	schedule_delayed_work(&mgr->decay_gc_work,
> +			      xe_ttm_vram_decay_gc_period(mgr));
> +	mutex_unlock(&mgr->lock);
> +}

[Severity: Low]
Does xe_ttm_vram_decay_gc_worker() need to reschedule mgr->decay_gc_work
unconditionally once it has been started?

After all XE_PAGE_OFFLINE_SOFT entries in mgr->offlined_pages have been
reinstated (or promoted to XE_PAGE_OFFLINE_HARD), or if!decay is taken,
jumping to reschedule keeps re-arming mgr->decay_gc_work every period until
driver teardown. Should the worker only reschedule itself when decay is
non-zero and at least one XE_PAGE_OFFLINE_SOFT entry still remains?

[ ... ]

> @@ -725,8 +776,12 @@ static bool xe_ttm_vram_page_already_processed(struct xe_ttm_vram_mgr *mgr,
>  	lockdep_assert_held(&mgr->lock);
>  
>  	list_for_each_entry(pos, &mgr->offlined_pages, offlined_link) {
> -		if (pos->addr == addr)
> +		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) {

[Severity: High]
As reported by Sashiko on RFC [1], what happens if a second hardware fault is
reported for the same address while the page's reservation is still deferred
on mgr->queued_pages?

In xe_ttm_vram_page_already_processed(), the second loop checks
mgr->queued_pages:

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

When a page was allocated to a BO at the time of the first fault (or its
buddy block was still busy), xe_ttm_vram_reserve_page_at_addr() places nentry
on mgr->queued_pages with mode = XE_PAGE_OFFLINE_SOFT while
xe_ttm_vram_purge_page() / TTM ghost object cleanup is still in flight.

If a second fault arrives before xe_ttm_vram_mgr_del() calls
xe_ttm_vram_retry_queued_pages(), xe_ttm_vram_page_already_processed() finds
the entry on mgr->queued_pages and returns true without promoting pos->mode
to XE_PAGE_OFFLINE_HARD or updating pos->last_seen. Once
xe_ttm_vram_retry_queued_pages() moves that entry to mgr->offlined_pages,
won't it remain in XE_PAGE_OFFLINE_SOFT mode and be reinstated after the
decay interval despite having faulted twice and being permanently retired in
firmware by handle_page_offline()?

> @@ -804,6 +859,8 @@ 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;
> +		nentry->last_seen = jiffies;
>  
>  		if (block) {
>  			pbo = xe_ttm_vram_block_owner_get(xe, block);

[Severity: High]
Won't this also mark permanently retired pages loaded from firmware at driver
probe as XE_PAGE_OFFLINE_SOFT and reinstate them after the decay window?

During probe, xe_ras_init() -> get_offline_list() -> process_offline_pages()
queries firmware for pages that were permanently retired on prior boots and
calls handle_page_offline(xe, common->page_addresses[i], false) once per
address:

drivers/gpu/drm/xe/xe_ras.c:process_offline_pages() {
	...
	for (i = 0; i < common->pages_returned && i < XE_RAS_NUM_PAGES; i++) {
		handle_page_offline(xe, common->page_addresses[i], notify_fw);
		/* The pages are already offlined by firmware */
		if (!notify_fw) {
			index = common->page_addresses[i] >> XE_PTE_SHIFT;
			ret = xa_err(xa_store(&state->offlined_pages, index,
					      XE_RAS_PAGE_OFFLINED, GFP_KERNEL));
	...
}

Because handle_page_offline() -> xe_ttm_vram_handle_addr_fault() ->
xe_ttm_vram_reserve_page_at_addr() is only called once per address at boot
when vram_mgr->offlined_pages is empty, nentry->mode is set to
XE_PAGE_OFFLINE_SOFT and decay_gc_work is scheduled. Once bad_page_decay_secs
elapses, xe_ttm_vram_decay_gc_worker() will free those permanently retired
pages back into the VRAM buddy allocator.

Should pages retired with notify_fw == false (or where firmware retirement has
already occurred) be initialized as XE_PAGE_OFFLINE_HARD?

> @@ -843,6 +900,11 @@ 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 &&
> +			    !delayed_work_pending(&vram_mgr->decay_gc_work)) {
> +				schedule_delayed_work(&vram_mgr->decay_gc_work,
> +						      xe_ttm_vram_decay_gc_period(vram_mgr));
> +			}
>  			return ret;
>  		}
>  	}

[Severity: High]
As reported by Sashiko on RFC [2], if a faulty page is currently allocated to
a BO (block != NULL) or xe_ttm_vram_buddy_alloc() fails because the block is
still busy, xe_ttm_vram_reserve_page_at_addr() queues nentry onto
vram_mgr->queued_pages without scheduling vram_mgr->decay_gc_work.

Later, when the BO is freed in xe_ttm_vram_mgr_del(),
xe_ttm_vram_retry_queued_pages() reserves the buddy block and moves the entry
to mgr->offlined_pages:

drivers/gpu/drm/xe/xe_ttm_vram_mgr.c:xe_ttm_vram_retry_queued_pages() {
	...
		--mgr->n_queued_pages;
		list_del(&pos->queued_link);
		++mgr->n_offlined_pages;
		list_add(&pos->offlined_link, &mgr->offlined_pages);
	}
}

Because xe_ttm_vram_retry_queued_pages() never schedules mgr->decay_gc_work,
won't soft-offlined pages that faulted while in use stay offline forever
unless an unallocated page happens to fault separately on the same manager?

[1] https://lore.kernel.org/r/20260911094616.1333590-2-tejas.upadhyay@intel.com
[2] https://lore.kernel.org/r/20260911094616.1333590-2-tejas.upadhyay@intel.com

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