Re: [Qemu-devel] [PATCH] rng-random: implement request queue

[Ladi Prosek]

On Thu, Mar 3, 2016 at 6:05 AM, Amit Shah <address@hidden> wrote:
> On (Thu) 04 Feb 2016 [13:07:35], Ladi Prosek wrote:
>> ----- Original Message -----
>> > ----- Original Message -----
>> > >
>> > >
>> > > On 03/02/2016 13:36, Amit Shah wrote:
>> > > > ... and this can lead to breaking migration (the queue of requests on
>> > > > the host needs to be migrated, else the new host will have no idea of
>> > > > the queue).
>> > >
>> > > It is already migrated as part of virtio_rng_save's call to virtio_save.
>> > >  On the loading side, virtio_rng_process condenses all requests into one
>> > > and chr_read fills in as many virtqueue buffers as possible from the
>> > > single request.
>> >
>> > Thanks! So this looks broken. The contract between virtio-rng and the rng
>> > backend is "one chr_read callback per one rng_backend_request_entropy",
>> > regardless of the request size. It guarantees that chr_read will get at
>> > least one byte, nothing else. If the one condensed request is bigger than
>> > what the backend is able to give at the moment, there may be unsatisfied
>> > virtio buffers left in the queue.
>> >
>> > One way of fixing this would be to have chr_read call virtio_rng_process
>> > if it ran out of backend data but not out of virtio buffers. Otherwise
>> > those buffers will be slowly drained by the rate limit timer, which is
>> > not optimal (especially in the absence of rate limit :)
>>
>> Basically, we could just revert this commit:
>>
>> commit 4621c1768ef5d12171cca2aa1473595ecb9f1c9e
>> Author: Amit Shah <address@hidden>
>> Date:   Wed Nov 21 11:21:19 2012 +0530
>>
>>     virtio-rng: remove extra request for entropy
>>
>>     If we got fewer bytes from the backend than requested, don't poke the
>>     backend for more bytes; the guest will ask for more (or if the guest has
>>     already asked for more, the backend knows about it via handle_input()
>
> OK, agreed.
>
> It makes sense to revert this so the live migration scenario is sane.
> Without live migration reverting this commit doesn't get us any
> benefit, and that's why I had removed it.
>
> Do you want to post a patch reverting this too?

Yes, I've had this ready for some time, just haven't posted it yet
because it depends on this series. Without this series reverting the
commit completes a nice loop in the callgraph chr_read ->
virtio_rng_process -> rng_backend_request_entropy -> chr_read, leading
to stack overflow. I've actually hit it in testing.

> Thanks,
>
>                 Amit