Quoting Tvrtko Ursulin (2018-05-10 17:15:46)
>
> On 10/05/2018 17:03, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-05-10 16:49:03)
> >>
> >> On 09/05/2018 15:27, Chris Wilson wrote:
> >>> 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@xxxxxxxxxxxxxxxxxx>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>> ---
> >>> +static inline void i915_tasklet_unlock(struct i915_tasklet *t)
> >>> +{
> >>> + tasklet_enable(&t->base);
> >>> +}
> >>
> >> I agree keeping the original naming for kill/enable/disable would be better.
> >>
> >>> +
> >>> +static inline void i915_tasklet_set_func(struct i915_tasklet *t,
> >>> + void (*func)(unsigned long data),
> >>> + unsigned long data)
> >>> +{
> >>> + i915_tasklet_lock(t);
> >>> +
> >>> + t->base.func = func;
> >>> + t->base.data = data;
> >>> +
> >>> + i915_tasklet_unlock(t);
> >>
> >> I kind of remember something about issues we had with placing
> >> tasklet_disable placed in some contexts. Hm.. if only I could recall the
> >> details. It's probably fine. I cannot come up with ideas on how to
> >> protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far
> >> fetched probably.
> >
> > When we get it wrong, CI complains with a lock up. It cannot be used in
> > atomic context as it uses yield() to kick the tasklet (as it may be on
> > the local cpu).
> >
> >>> @@ -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);
> >>
> >> Or eliminate the above dilemma by removing the data parameter from
> >> i915_tasklet_set_func since it looks it is not needed at the moment?
> >
> > It doesn't eliminate the dilemma, updating the func itself raises the
> > question of what if the tasklet is running at that time, what was the
> > caller thinking would happen?
>
> Well same as it can do now so you could drop tasklet_disable/enable
> then. At least I assumed the purpose is for atomicity of func/data updates.
Sure, it mostly there to catch using it from silly circumstances where
the tasklet may be being called as the change occurs. Should never
happen, so I bet it does.
> > I expect tasklets will start passing themselves to the func and
> > eliminate the data parameters themselves at some point in the mid term.
>
> Needs to be tasklet_disable_nosync then, or it will hang. But then it
> loses its purpose anyway. So I am confused.
I was just referring to my hope that the unsigned long data parameter
will be moved to the great dustbin of history. If timers can make the
leap, so can tasklets!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
