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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> +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.

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

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx