From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: Date: Fri, 5 May 2023 16:37:52 +0200 MIME-Version: 1.0 References: <20230411150515.14020-1-hreitz@redhat.com> <71e47e3e-880d-38d8-c1b0-3287c60365e4@redhat.com> From: Hanna Czenczek In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Subject: Re: [Virtio-fs] [PATCH 0/4] vhost-user-fs: Internal migration List-Id: Development discussions about virtio-fs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eugenio Perez Martin Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, virtio-fs@redhat.com, Stefan Hajnoczi , German Maglione , Anton Kuchin , Juan Quintela , "Michael S . Tsirkin" , Stefano Garzarella On 05.05.23 16:26, Eugenio Perez Martin wrote: > On Fri, May 5, 2023 at 11:51 AM Hanna Czenczek wrote: >> (By the way, thanks for the explanations :)) >> >> On 05.05.23 11:03, Hanna Czenczek wrote: >>> On 04.05.23 23:14, Stefan Hajnoczi wrote: >> [...] >> >>>> I think it's better to change QEMU's vhost code >>>> to leave stateful devices suspended (but not reset) across >>>> vhost_dev_stop() -> vhost_dev_start(), maybe by introducing >>>> vhost_dev_suspend() and vhost_dev_resume(). Have you thought about >>>> this aspect? >>> Yes and no; I mean, I haven’t in detail, but I thought this is what’s >>> meant by suspending instead of resetting when the VM is stopped. >> So, now looking at vhost_dev_stop(), one problem I can see is that >> depending on the back-end, different operations it does will do >> different things. >> >> It tries to stop the whole device via vhost_ops->vhost_dev_start(), >> which for vDPA will suspend the device, but for vhost-user will reset it >> (if F_STATUS is there). >> >> It disables all vrings, which doesn’t mean stopping, but may be >> necessary, too. (I haven’t yet really understood the use of disabled >> vrings, I heard that virtio-net would have a need for it.) >> >> It then also stops all vrings, though, so that’s OK. And because this >> will always do GET_VRING_BASE, this is actually always the same >> regardless of transport. >> >> Finally (for this purpose), it resets the device status via >> vhost_ops->vhost_reset_status(). This is only implemented on vDPA, and >> this is what resets the device there. >> >> >> So vhost-user resets the device in .vhost_dev_start, but vDPA only does >> so in .vhost_reset_status. It would seem better to me if vhost-user >> would also reset the device only in .vhost_reset_status, not in >> .vhost_dev_start. .vhost_dev_start seems precisely like the place to >> run SUSPEND/RESUME. >> > I think the same. I just saw It's been proposed at [1]. > >> Another question I have (but this is basically what I wrote in my last >> email) is why we even call .vhost_reset_status here. If the device >> and/or all of the vrings are already stopped, why do we need to reset >> it? Naïvely, I had assumed we only really need to reset the device if >> the guest changes, so that a new guest driver sees a freshly initialized >> device. >> > I don't know why we didn't need to call it :). I'm assuming the > previous vhost-user net did fine resetting vq indexes, using > VHOST_USER_SET_VRING_BASE. But I don't know about more complex > devices. > > The guest can reset the device, or write 0 to the PCI config status, > at any time. How does virtiofs handle it, being stateful? Honestly a good question because virtiofsd implements neither SET_STATUS nor RESET_DEVICE.  I’ll have to investigate that. I think when the guest resets the device, SET_VRING_BASE always comes along some way or another, so that’s how the vrings are reset.  Maybe the internal state is reset only following more high-level FUSE commands like INIT. Hanna