Re: [Qemu-devel] [PATCH] dma: Avoid reentrancy in DMA transfer handlers

[Kevin Wolf]

Am 31.10.2011 17:00, schrieb Kevin Wolf:
> Am 31.10.2011 16:34, schrieb Paolo Bonzini:
>> On 10/31/2011 03:46 PM, Kevin Wolf wrote:
>>>>  Hmm, I think you should set rearm = 1 to ensure the BH is run when
>>>>  ultimately you leave the sync read.  Sorry for not spotting this before.
>>>
>>> I was about to agree, but in fact adding a rearm = 1; line leads to
>>> crashes, whereas in the version I posted it just works. So it looks like
>>> something is wrong with doing it, even though it seemed to make perfect
>>> sense at the first sight.
>>
>> But what will restart the DMA at the end of the synchronous I/O, then?
> 
> bdrv_read/write are called inside fdctrl_read_data(), so the outer
> DMA_run() already has rearm = 1.
> 
> I think the more interesting question is why rescheduling can break
> anything. Where would we schedule the BH additionally when it isn't
> already scheduled anyway?

I think I found the problem:

> @@ -374,6 +381,8 @@ static void DMA_run (void)
>          }
>      }
>  
> +out:
> +    running = 0;
>      if (rearm)
>          qemu_bh_schedule_idle(dma_bh);
>  }

We should only reset running to 0 in the outermost instance. Moving the
out: label a line down seems to fix the crashes.

Kevin