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

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

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@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.
> 
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx