All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Ladi Prosek <lprosek@redhat.com>
Cc: Amit Shah <amit.shah@redhat.com>,
	pagupta@redhat.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] rng-random: implement request queue
Date: Fri, 5 Feb 2016 09:32:01 +0100	[thread overview]
Message-ID: <56B45E01.3090203@redhat.com> (raw)
In-Reply-To: <65843372.32316615.1454609255745.JavaMail.zimbra@redhat.com>


>>> 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 :)

Nice observation!  And the commit you mention below seems to be the
right way to fix it indeed.  Amit, do you recall why this was done?

> Basically, we could just revert this commit:
> 
> commit 4621c1768ef5d12171cca2aa1473595ecb9f1c9e
> Author: Amit Shah <amit.shah@redhat.com>
> 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()

I've now reviewed your patch more carefully, I will make some comments
in reply to the patch directly.

Paolo

  reply	other threads:[~2016-02-05  8:32 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-22 12:19 [Qemu-devel] [PATCH] rng-random: implement request queue Ladi Prosek
2016-02-03 12:36 ` Amit Shah
2016-02-03 18:02   ` Ladi Prosek
2016-02-03 18:44   ` Paolo Bonzini
2016-02-04  8:53     ` Pankaj Gupta
2016-02-04 17:36       ` Ladi Prosek
2016-02-05  5:31         ` Pankaj Gupta
2016-02-04 17:24     ` Ladi Prosek
2016-02-04 18:07       ` Ladi Prosek
2016-02-05  8:32         ` Paolo Bonzini [this message]
2016-03-03  5:05         ` Amit Shah
2016-03-03  9:30           ` Ladi Prosek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56B45E01.3090203@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=amit.shah@redhat.com \
    --cc=lprosek@redhat.com \
    --cc=pagupta@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.