Re: [PATCH 3/3] drm/sched: Remove a hole from struct drm_sched_job

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Feb 06, 2025 at 04:40:31PM +0000, Tvrtko Ursulin wrote:
> We can re-order some struct members and take u32 credits outside of the
> pointer sandwich and also for the last_dependency member we can get away
> with an unsigned int since for dependency we use xa_limit_32b.
> 
> Pahole report before:
>         /* size: 160, cachelines: 3, members: 14 */
>         /* sum members: 156, holes: 1, sum holes: 4 */
>         /* last cacheline: 32 bytes */
> 
> And after:
>         /* size: 152, cachelines: 3, members: 14 */
>         /* last cacheline: 24 bytes */
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>  include/drm/gpu_scheduler.h | 38 +++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index a0ff08123f07..68da3dec8dba 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -338,8 +338,14 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
>   * to schedule the job.
>   */
>  struct drm_sched_job {
> -	struct spsc_node		queue_node;
> -	struct list_head		list;
> +	u64				id;
> +
> +	/**
> +	 * @submit_ts:
> +	 *
> +	 * When the job was pushed into the entity queue.
> +	 */
> +	ktime_t                         submit_ts;
>  
>  	/**
>  	 * @sched:
> @@ -349,24 +355,30 @@ struct drm_sched_job {
>  	 * has finished.
>  	 */
>  	struct drm_gpu_scheduler	*sched;
> +
>  	struct drm_sched_fence		*s_fence;
> +	struct drm_sched_entity         *entity;
>  
> +	enum drm_sched_priority		s_priority;
>  	u32				credits;
> +	/** @last_dependency: tracks @dependencies as they signal */
> +	unsigned int			last_dependency;
> +	atomic_t			karma;
> +
> +	struct spsc_node		queue_node;
> +	struct list_head		list;
>  
>  	/*
>  	 * work is used only after finish_cb has been used and will not be
>  	 * accessed anymore.
>  	 */
>  	union {
> -		struct dma_fence_cb		finish_cb;
> -		struct work_struct		work;
> +		struct dma_fence_cb	finish_cb;
> +		struct work_struct	work;

I usually prefer to leave those things alone, but since you change most of this
struct anyways...

With or without this diff:

Acked-by: Danilo Krummrich <dakr@kernel.org>

>  	};
>  
> -	uint64_t			id;
> -	atomic_t			karma;
> -	enum drm_sched_priority		s_priority;
> -	struct drm_sched_entity         *entity;
>  	struct dma_fence_cb		cb;
> +
>  	/**
>  	 * @dependencies:
>  	 *
> @@ -375,16 +387,6 @@ struct drm_sched_job {
>  	 * drm_sched_job_add_implicit_dependencies().
>  	 */
>  	struct xarray			dependencies;
> -
> -	/** @last_dependency: tracks @dependencies as they signal */
> -	unsigned long			last_dependency;
> -
> -	/**
> -	 * @submit_ts:
> -	 *
> -	 * When the job was pushed into the entity queue.
> -	 */
> -	ktime_t                         submit_ts;
>  };
>  
>  static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
> -- 
> 2.48.0
>