On 10/05/2018 17:19, Chris Wilson wrote:
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.
You could do similar by asserting tasklet is either disabled, or at
least not running.
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!
I just don't see the point in adding the second parameter now. I mean
it's not a big deal, but I don't see it useful.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
