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

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Mika Kuoppala (2018-05-09 14:44:30)
> 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() ?

Because I didn't want to resend the series immediately. It's confusing
because it's set by tasklet_trylock and cleared by tasklet_unlock.

> > +     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.

Oh, I wouldn't worry about that, just wait until you see what comes
later :-p

> > +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?
> 

I was thinking about being clear about the ordering.

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

As you point out, the lock is tasklet_disable, the trylock is just a
trylock. Embrace the insanity, let it flow through you.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx