From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43521) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abPaB-000331-Ov for qemu-devel@nongnu.org; Thu, 03 Mar 2016 04:30:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abPa8-00027m-JV for qemu-devel@nongnu.org; Thu, 03 Mar 2016 04:30:07 -0500 Received: from mail-vk0-f46.google.com ([209.85.213.46]:32772) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abPa8-00027O-FS for qemu-devel@nongnu.org; Thu, 03 Mar 2016 04:30:04 -0500 Received: by mail-vk0-f46.google.com with SMTP id k196so15714588vka.0 for ; Thu, 03 Mar 2016 01:30:04 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20160303050538.GD15443@grmbl.mre> References: <1453465198-11000-1-git-send-email-lprosek@redhat.com> <20160203123639.GA20527@grmbl.mre> <56B24A9C.9010104@redhat.com> <725955967.32271042.1454606645414.JavaMail.zimbra@redhat.com> <65843372.32316615.1454609255745.JavaMail.zimbra@redhat.com> <20160303050538.GD15443@grmbl.mre> Date: Thu, 3 Mar 2016 10:30:03 +0100 Message-ID: From: Ladi Prosek Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH] rng-random: implement request queue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: Paolo Bonzini , qemu-devel@nongnu.org, pagupta On Thu, Mar 3, 2016 at 6:05 AM, Amit Shah 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 >> 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