Re: [RFC 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 4 potential issue(s) to consider:
- [High] Secondary faults on queued pages are ignored, failing to promote them to HARD offline mode or update their timestamp.
- [High] The decay timer is not armed when queued pages are successfully soft-offlined, potentially leaving them offline forever.
- [Medium] The new configfs parameter 'bad_page_decay_secs' is ignored, causing the decay timer to always use the hardcoded default.
- [Low] Decay intervals larger than MAX_JIFFY_OFFSET will cause pages to be instantly reinstated due to time_after() wrap-around on 32-bit platforms.
--

commit 2fea07001f29f1688568140a8cffb0e02a08838d
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>

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

This commit introduces a decay timer for soft-offlined VRAM pages, allowing
them to be reinstated after a configurable period without further faults.
It adds tracking for soft/hard offline modes, promoting pages to hard
offline on a second fault. Configuration is exposed via configfs.

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index f5c828cf7e8f2..d866eb93f05ff 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
[ ... ]
> +static ssize_t bad_page_decay_secs_store(struct config_item *item, const char *page, size_t len)
> +{
> +	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(page, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Cap at ~68 years to keep secs_to_jiffies() sane on 32-bit HZ=1000. */
> +	if (val > (U32_MAX / 1000))
> +		return -EINVAL;

[Severity: Low]
Could this validation allow values that cause wrap-around on 32-bit platforms?
Values exceeding MAX_JIFFY_OFFSET (approx 24.8 days on 32-bit systems with
HZ=1000) will cause time_after() to immediately return true, instantly
reinstating the page instead of waiting.

[ ... ]
> +u32 xe_configfs_get_bad_page_decay_secs(struct pci_dev *pdev)
> +{
> +	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> +	u32 ret;
> +
> +	if (!dev)
> +		return device_defaults.bad_page_decay_secs;
> +
> +	ret = dev->config.bad_page_decay_secs;
> +	config_group_put(&dev->group);
> +
> +	return ret;
> +}

[Severity: Medium]
This accessor is added to retrieve the user's setting, but does the driver
ever call it? The initialization logic appears to ignore this and hardcodes
the default macro instead.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 9a514d983e90b..c1bfb9bc826ce 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
[ ... ]
> @@ -446,6 +493,9 @@ int __xe_ttm_vram_mgr_init(struct xe_device *xe, struct xe_ttm_vram_mgr *mgr,
>  		return err;
>  	INIT_LIST_HEAD(&mgr->offlined_pages);
>  	INIT_LIST_HEAD(&mgr->queued_pages);
> +	INIT_DELAYED_WORK(&mgr->decay_gc_work, xe_ttm_vram_decay_gc_worker);
> +	mgr->decay_interval_jiffies =
> +		secs_to_jiffies(XE_BAD_PAGE_DECAY_SECS_DEFAULT);

[Severity: Medium]
Should this be calling xe_configfs_get_bad_page_decay_secs() instead of using
the hardcoded default macro? As currently written, the configfs parameter
seems to be completely ignored.

[ ... ]
> @@ -711,8 +761,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) {
>  		if (pos->addr == addr)
>  			return true;
>  	}

[Severity: High]
Does the queued_pages loop also need to promote the page mode to
XE_PAGE_OFFLINE_HARD and update the timestamp for secondary faults?
Without this, pages that fault repeatedly while they are occupied and in the
queued_pages list might eventually decay and be reinstated, which bypasses
the two-strike permanent retirement policy.

[ ... ]
> @@ -790,6 +844,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);
[ ... ]
>  			/* Queue free(to-be-purged) pages */
>  			++vram_mgr->n_queued_pages;
>  			list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
>  		} else {
>  			/* Immediately offline unoccupied pages */
[ ... ]
>  			++vram_mgr->n_offlined_pages;
>  			list_add_rcu(&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]
When occupied pages are added to the queued_pages list in the upper block,
the decay timer is not armed. If all initial hardware faults occur on
occupied pages, could this result in the timer never starting? It looks
like the decay worker is only scheduled when unoccupied pages are
successfully offlined in the lower block, which might leave queued pages
offline forever once they are eventually purged.

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