[Intel-gfx] [PATCH] drm/i915/gt: Carefully order virtual_submission_tasklet

[Tvrtko Ursulin]

On 23/04/2020 12:53, Chris Wilson wrote:
> During the virtual engine's submission tasklet, we take the request and
> insert into the submission queue on each of our siblings. This seems
> quite simply, and so no problems with ordering. However, the sibling
> execlists' submission tasklets may run concurrently with the virtual
> engine's tasklet, submitting the request to HW before the virtual
> finishes its task of telling all the siblings. If this happens, the
> sibling tasklet may *reorder* the ve->sibling[] array that the virtual
> engine tasklet is processing. This can *only* reorder within the
> elements already processed by the virtual engine, nevertheless the
> race is detected by KCSAN:
> 
> [  185.580014] BUG: KCSAN: data-race in execlists_dequeue [i915] / virtual_submission_tasklet [i915]
> [  185.580054]
> [  185.580076] write to 0xffff8881f1919860 of 8 bytes by interrupt on cpu 2:
> [  185.580553]  execlists_dequeue+0x6ad/0x1600 [i915]
> [  185.581044]  __execlists_submission_tasklet+0x48/0x60 [i915]
> [  185.581517]  execlists_submission_tasklet+0xd3/0x170 [i915]
> [  185.581554]  tasklet_action_common.isra.0+0x42/0x90
> [  185.581585]  __do_softirq+0xc8/0x206
> [  185.581613]  run_ksoftirqd+0x15/0x20
> [  185.581641]  smpboot_thread_fn+0x15a/0x270
> [  185.581669]  kthread+0x19a/0x1e0
> [  185.581695]  ret_from_fork+0x1f/0x30
> [  185.581717]
> [  185.581736] read to 0xffff8881f1919860 of 8 bytes by interrupt on cpu 0:
> [  185.582231]  virtual_submission_tasklet+0x10e/0x5c0 [i915]
> [  185.582265]  tasklet_action_common.isra.0+0x42/0x90
> [  185.582291]  __do_softirq+0xc8/0x206
> [  185.582315]  run_ksoftirqd+0x15/0x20
> [  185.582340]  smpboot_thread_fn+0x15a/0x270
> [  185.582368]  kthread+0x19a/0x1e0
> [  185.582395]  ret_from_fork+0x1f/0x30
> [  185.582417]
> 
> We can prevent this race by checking for the ve->request after looking
> up the sibling array.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index fba774a0abbf..dead24aaf45d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -5092,12 +5092,15 @@ static void virtual_submission_tasklet(unsigned long data)
>   		return;
>   
>   	local_irq_disable();
> -	for (n = 0; READ_ONCE(ve->request) && n < ve->num_siblings; n++) {
> -		struct intel_engine_cs *sibling = ve->siblings[n];
> +	for (n = 0; n < ve->num_siblings; n++) {
> +		struct intel_engine_cs *sibling = READ_ONCE(ve->siblings[n]);
>   		struct ve_node * const node = &ve->nodes[sibling->id];
>   		struct rb_node **parent, *rb;
>   		bool first;
>   
> +		if (!READ_ONCE(ve->request))
> +			break; /* already handled by a sibling's tasklet */
> +
>   		if (unlikely(!(mask & sibling->mask))) {
>   			if (!RB_EMPTY_NODE(&node->rb)) {
>   				spin_lock(&sibling->active.lock);
> 

Useful debugging aid!

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko