Re: [PATCH v3 1/5] drm/i915: Wrap tasklet_struct for abuse

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> In the next few patches, we want to abuse tasklet to avoid ksoftirqd
> latency along critical paths. To make that abuse easily to swallow,
> first coat the tasklet in a little syntactic sugar.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c             |  8 +--
>  drivers/gpu/drm/i915/i915_irq.c             |  2 +-
>  drivers/gpu/drm/i915/i915_tasklet.h         | 78 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_engine_cs.c      | 11 ++-
>  drivers/gpu/drm/i915/intel_guc_submission.c |  8 +--
>  drivers/gpu/drm/i915/intel_lrc.c            | 18 ++---
>  drivers/gpu/drm/i915/intel_ringbuffer.h     |  3 +-
>  7 files changed, 104 insertions(+), 24 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_tasklet.h
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 89bf5d67cb74..98481e150e61 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3043,9 +3043,9 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
>  	 * common case of recursively being called from set-wedged from inside
>  	 * i915_reset.
>  	 */
> -	if (!atomic_read(&engine->execlists.tasklet.count))
> -		tasklet_kill(&engine->execlists.tasklet);
> -	tasklet_disable(&engine->execlists.tasklet);
> +	if (i915_tasklet_is_enabled(&engine->execlists.tasklet))
> +		i915_tasklet_flush(&engine->execlists.tasklet);
> +	i915_tasklet_disable(&engine->execlists.tasklet);
>  
>  	/*
>  	 * We're using worker to queue preemption requests from the tasklet in
> @@ -3265,7 +3265,7 @@ void i915_gem_reset(struct drm_i915_private *dev_priv,
>  
>  void i915_gem_reset_finish_engine(struct intel_engine_cs *engine)
>  {
> -	tasklet_enable(&engine->execlists.tasklet);
> +	i915_tasklet_enable(&engine->execlists.tasklet);
>  	kthread_unpark(engine->breadcrumbs.signaler);
>  
>  	intel_uncore_forcewake_put(engine->i915, FORCEWAKE_ALL);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f9bc3aaa90d0..f8aff5a5aa83 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1477,7 +1477,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
>  	}
>  
>  	if (tasklet)
> -		tasklet_hi_schedule(&execlists->tasklet);
> +		i915_tasklet_schedule(&execlists->tasklet);
>  }
>  
>  static void gen8_gt_irq_ack(struct drm_i915_private *i915,
> diff --git a/drivers/gpu/drm/i915/i915_tasklet.h b/drivers/gpu/drm/i915/i915_tasklet.h
> new file mode 100644
> index 000000000000..c9f41a5ebb91
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_tasklet.h
> @@ -0,0 +1,78 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright © 2018 Intel Corporation
> + */
> +
> +#ifndef _I915_TASKLET_H_
> +#define _I915_TASKLET_H_
> +
> +#include <linux/atomic.h>
> +#include <linux/interrupt.h>
> +
> +/**
> + * struct i915_tasklet - wrapper around tasklet_struct
> + *
> + * We want to abuse tasklets slightly, such as calling them directly. In some
> + * cases, this requires some auxiliary tracking so subclass the tasklet_struct
> + * so that we have a central place and helpers.
> + */
> +struct i915_tasklet {
> +	struct tasklet_struct base;
> +};
> +
> +static inline void i915_tasklet_init(struct i915_tasklet *t,
> +				     void (*func)(unsigned long),
> +				     unsigned long data)
> +{
> +	tasklet_init(&t->base, func, data);
> +}
> +
> +static inline bool i915_tasklet_is_scheduled(const struct i915_tasklet *t)
> +{
> +	return test_bit(TASKLET_STATE_SCHED, &t->base.state);
> +}
> +
> +static inline bool i915_tasklet_is_locked(const struct i915_tasklet *t)

why not is_running() ?

> +{
> +	return test_bit(TASKLET_STATE_RUN, &t->base.state);
> +}
> +
> +static inline bool i915_tasklet_is_enabled(const struct i915_tasklet *t)
> +{
> +	return likely(!atomic_read(&t->base.count));

I am concerned that we bury the sign of racy nature out of sight and
then it comes and bites us.

-Mika

> +}
> +
> +static inline void i915_tasklet_schedule(struct i915_tasklet *t)
> +{
> +	tasklet_hi_schedule(&t->base);
> +}
> +
> +static inline void i915_tasklet_flush(struct i915_tasklet *t)
> +{
> +	tasklet_kill(&t->base);
> +}
> +
> +static inline void i915_tasklet_enable(struct i915_tasklet *t)
> +{
> +	tasklet_enable(&t->base);
> +}
> +
> +static inline void i915_tasklet_disable(struct i915_tasklet *t)
> +{
> +	tasklet_disable(&t->base);
> +}
> +
> +static inline void i915_tasklet_set_func(struct i915_tasklet *t,
> +					 void (*func)(unsigned long data),
> +					 unsigned long data)
> +{
> +	tasklet_disable(&t->base);

What does the disable/enable pair buy us here?

It looks that you want trylock and unlock
so that you can safely change the func/data.

-Mika

> +
> +	t->base.func = func;
> +	t->base.data = data;
> +
> +	tasklet_enable(&t->base);
> +}
> +
> +#endif /* _I915_TASKLET_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 70325e0824e3..5f1118ea96d8 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1032,7 +1032,7 @@ void intel_engines_park(struct drm_i915_private *i915)
>  	for_each_engine(engine, i915, id) {
>  		/* Flush the residual irq tasklets first. */
>  		intel_engine_disarm_breadcrumbs(engine);
> -		tasklet_kill(&engine->execlists.tasklet);
> +		i915_tasklet_flush(&engine->execlists.tasklet);
>  
>  		/*
>  		 * We are committed now to parking the engines, make sure there
> @@ -1249,9 +1249,8 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
>  			   intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)),
>  			   yesno(test_bit(ENGINE_IRQ_EXECLIST,
>  					  &engine->irq_posted)),
> -			   yesno(test_bit(TASKLET_STATE_SCHED,
> -					  &engine->execlists.tasklet.state)),
> -			   enableddisabled(!atomic_read(&engine->execlists.tasklet.count)));
> +			   yesno(i915_tasklet_is_scheduled(&engine->execlists.tasklet)),
> +			   enableddisabled(i915_tasklet_is_enabled(&engine->execlists.tasklet)));
>  		if (read >= GEN8_CSB_ENTRIES)
>  			read = 0;
>  		if (write >= GEN8_CSB_ENTRIES)
> @@ -1479,7 +1478,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>  	if (!intel_engine_supports_stats(engine))
>  		return -ENODEV;
>  
> -	tasklet_disable(&execlists->tasklet);
> +	i915_tasklet_disable(&execlists->tasklet);
>  	write_seqlock_irqsave(&engine->stats.lock, flags);
>  
>  	if (unlikely(engine->stats.enabled == ~0)) {
> @@ -1505,7 +1504,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>  
>  unlock:
>  	write_sequnlock_irqrestore(&engine->stats.lock, flags);
> -	tasklet_enable(&execlists->tasklet);
> +	i915_tasklet_enable(&execlists->tasklet);
>  
>  	return err;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 2feb65096966..a7afc976c3b9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -591,7 +591,7 @@ static void inject_preempt_context(struct work_struct *work)
>  	if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {
>  		execlists_clear_active(&engine->execlists,
>  				       EXECLISTS_ACTIVE_PREEMPT);
> -		tasklet_schedule(&engine->execlists.tasklet);
> +		i915_tasklet_schedule(&engine->execlists.tasklet);
>  	}
>  }
>  
> @@ -1263,10 +1263,10 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>  	guc_interrupts_capture(dev_priv);
>  
>  	for_each_engine(engine, dev_priv, id) {
> -		struct intel_engine_execlists * const execlists =
> -			&engine->execlists;
> +		i915_tasklet_set_func(&engine->execlists.tasklet,
> +				      guc_submission_tasklet,
> +				      (unsigned long)engine);
>  
> -		execlists->tasklet.func = guc_submission_tasklet;
>  		engine->park = guc_submission_park;
>  		engine->unpark = guc_submission_unpark;
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 7f98dda3c929..539fa03d7600 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1165,7 +1165,7 @@ static void queue_request(struct intel_engine_cs *engine,
>  static void __submit_queue(struct intel_engine_cs *engine, int prio)
>  {
>  	engine->execlists.queue_priority = prio;
> -	tasklet_hi_schedule(&engine->execlists.tasklet);
> +	i915_tasklet_schedule(&engine->execlists.tasklet);
>  }
>  
>  static void submit_queue(struct intel_engine_cs *engine, int prio)
> @@ -1778,7 +1778,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  
>  	/* After a GPU reset, we may have requests to replay */
>  	if (execlists->first)
> -		tasklet_schedule(&execlists->tasklet);
> +		i915_tasklet_schedule(&execlists->tasklet);
>  
>  	return 0;
>  }
> @@ -2182,9 +2182,8 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
>  	 * Tasklet cannot be active at this point due intel_mark_active/idle
>  	 * so this is just for documentation.
>  	 */
> -	if (WARN_ON(test_bit(TASKLET_STATE_SCHED,
> -			     &engine->execlists.tasklet.state)))
> -		tasklet_kill(&engine->execlists.tasklet);
> +	if (WARN_ON(i915_tasklet_is_scheduled(&engine->execlists.tasklet)))
> +		i915_tasklet_flush(&engine->execlists.tasklet);
>  
>  	dev_priv = engine->i915;
>  
> @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
>  	engine->submit_request = execlists_submit_request;
>  	engine->cancel_requests = execlists_cancel_requests;
>  	engine->schedule = execlists_schedule;
> -	engine->execlists.tasklet.func = execlists_submission_tasklet;
> +
> +	i915_tasklet_set_func(&engine->execlists.tasklet,
> +			      execlists_submission_tasklet,
> +			      (unsigned long)engine);
>  
>  	engine->park = NULL;
>  	engine->unpark = NULL;
> @@ -2303,8 +2305,8 @@ logical_ring_setup(struct intel_engine_cs *engine)
>  
>  	engine->execlists.fw_domains = fw_domains;
>  
> -	tasklet_init(&engine->execlists.tasklet,
> -		     execlists_submission_tasklet, (unsigned long)engine);
> +	i915_tasklet_init(&engine->execlists.tasklet,
> +			  execlists_submission_tasklet, (unsigned long)engine);
>  
>  	logical_ring_default_vfuncs(engine);
>  	logical_ring_default_irqs(engine);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 010750e8ee44..f6ba354faf89 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -11,6 +11,7 @@
>  #include "i915_pmu.h"
>  #include "i915_request.h"
>  #include "i915_selftest.h"
> +#include "i915_tasklet.h"
>  #include "i915_timeline.h"
>  #include "intel_gpu_commands.h"
>  
> @@ -202,7 +203,7 @@ struct intel_engine_execlists {
>  	/**
>  	 * @tasklet: softirq tasklet for bottom handler
>  	 */
> -	struct tasklet_struct tasklet;
> +	struct i915_tasklet tasklet;
>  
>  	/**
>  	 * @default_priolist: priority list for I915_PRIORITY_NORMAL
> -- 
> 2.17.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx