Re: [PATCH v5 2/3] drm/xe: Move the coredump registration to the worker thread

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Nov 26, 2024 at 11:18:56AM -0800, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Adding lockdep checking to the coredump code showed that there was an
> existing violation. The dev_coredumpm_timeout() call is used to
> register the dump with the base coredump subsystem. However, that
> makes multiple memory allocations, only some of which use the GFP_
> flags passed in. So that also needs to be deferred to the worker
> function where it is safe to allocate with arbitrary flags.
> 
> In order to not add protoypes for the callback functions, moving the
> _timeout call also means moving the worker thread function to later in
> the file.
> 

I'd add a fixes tag + CC stable kernel when merging as we could deadlock
overselves if an devcoredump was done under extreme memory pressure.

Otherwise LGTM:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>  

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/xe/xe_devcoredump.c | 63 ++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index f4c77f525819..5d19a4e3d5af 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -167,31 +167,6 @@ static void xe_devcoredump_snapshot_free(struct xe_devcoredump_snapshot *ss)
>  	ss->vm = NULL;
>  }
>  
> -static void xe_devcoredump_deferred_snap_work(struct work_struct *work)
> -{
> -	struct xe_devcoredump_snapshot *ss = container_of(work, typeof(*ss), work);
> -	struct xe_devcoredump *coredump = container_of(ss, typeof(*coredump), snapshot);
> -	unsigned int fw_ref;
> -
> -	/* keep going if fw fails as we still want to save the memory and SW data */
> -	fw_ref = xe_force_wake_get(gt_to_fw(ss->gt), XE_FORCEWAKE_ALL);
> -	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL))
> -		xe_gt_info(ss->gt, "failed to get forcewake for coredump capture\n");
> -	xe_vm_snapshot_capture_delayed(ss->vm);
> -	xe_guc_exec_queue_snapshot_capture_delayed(ss->ge);
> -	xe_force_wake_put(gt_to_fw(ss->gt), fw_ref);
> -
> -	/* Calculate devcoredump size */
> -	ss->read.size = __xe_devcoredump_read(NULL, INT_MAX, coredump);
> -
> -	ss->read.buffer = kvmalloc(ss->read.size, GFP_USER);
> -	if (!ss->read.buffer)
> -		return;
> -
> -	__xe_devcoredump_read(ss->read.buffer, ss->read.size, coredump);
> -	xe_devcoredump_snapshot_free(ss);
> -}
> -
>  static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
>  				   size_t count, void *data, size_t datalen)
>  {
> @@ -240,6 +215,40 @@ static void xe_devcoredump_free(void *data)
>  		 "Xe device coredump has been deleted.\n");
>  }
>  
> +static void xe_devcoredump_deferred_snap_work(struct work_struct *work)
> +{
> +	struct xe_devcoredump_snapshot *ss = container_of(work, typeof(*ss), work);
> +	struct xe_devcoredump *coredump = container_of(ss, typeof(*coredump), snapshot);
> +	unsigned int fw_ref;
> +
> +	/*
> +	 * NB: Despite passing a GFP_ flags parameter here, more allocations are done
> +	 * internally using GFP_KERNEL expliictly. Hence this call must be in the worker
> +	 * thread and not in the initial capture call.
> +	 */
> +	dev_coredumpm_timeout(gt_to_xe(ss->gt)->drm.dev, THIS_MODULE, coredump, 0, GFP_KERNEL,
> +			      xe_devcoredump_read, xe_devcoredump_free,
> +			      XE_COREDUMP_TIMEOUT_JIFFIES);
> +
> +	/* keep going if fw fails as we still want to save the memory and SW data */
> +	fw_ref = xe_force_wake_get(gt_to_fw(ss->gt), XE_FORCEWAKE_ALL);
> +	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL))
> +		xe_gt_info(ss->gt, "failed to get forcewake for coredump capture\n");
> +	xe_vm_snapshot_capture_delayed(ss->vm);
> +	xe_guc_exec_queue_snapshot_capture_delayed(ss->ge);
> +	xe_force_wake_put(gt_to_fw(ss->gt), fw_ref);
> +
> +	/* Calculate devcoredump size */
> +	ss->read.size = __xe_devcoredump_read(NULL, INT_MAX, coredump);
> +
> +	ss->read.buffer = kvmalloc(ss->read.size, GFP_USER);
> +	if (!ss->read.buffer)
> +		return;
> +
> +	__xe_devcoredump_read(ss->read.buffer, ss->read.size, coredump);
> +	xe_devcoredump_snapshot_free(ss);
> +}
> +
>  static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>  				 struct xe_exec_queue *q,
>  				 struct xe_sched_job *job)
> @@ -328,10 +337,6 @@ void xe_devcoredump(struct xe_exec_queue *q, struct xe_sched_job *job, const cha
>  	drm_info(&xe->drm, "Xe device coredump has been created\n");
>  	drm_info(&xe->drm, "Check your /sys/class/drm/card%d/device/devcoredump/data\n",
>  		 xe->drm.primary->index);
> -
> -	dev_coredumpm_timeout(xe->drm.dev, THIS_MODULE, coredump, 0, GFP_KERNEL,
> -			      xe_devcoredump_read, xe_devcoredump_free,
> -			      XE_COREDUMP_TIMEOUT_JIFFIES);
>  }
>  
>  static void xe_driver_devcoredump_fini(void *arg)
> -- 
> 2.47.0
>