On 10.07.24 15:39, Matias Ezequiel Vara Larsen wrote:
Hello Hanna,
On Wed, Jul 10, 2024 at 01:23:10PM +0200, Hanna Czenczek wrote:
Requiring `vhost_started` to be true for resetting vhost devices in
`virtio_reset()` seems like the wrong condition: Most importantly, the
preceding `virtio_set_status(vdev, 0)` call will (for vhost devices) end
up in `vhost_dev_stop()` (through vhost devices' `.set_status`
implementations), setting `vdev->vhost_started = false`. Therefore, the
gated `vhost_reset_device()` call is unreachable.
`vhost_started` is not documented, so it is hard to say what exactly it
is supposed to mean, but judging from the fact that `vhost_dev_start()`
sets it and `vhost_dev_stop()` clears it, it seems like it indicates
whether there is a vhost back-end, and whether that back-end is
currently running and processing virtio requests.
Making a reset conditional on whether the vhost back-end is processing
virtio requests seems wrong; in fact, it is probably better to reset it
only when it is not currently processing requests, which is exactly the
current order of operations in `virtio_reset()`: First, the back-end is
stopped through `virtio_set_status(vdev, 0)`, then we want to send a
reset.
Therefore, we should drop the `vhost_started` condition, but in its
stead we then have to verify that we can indeed send a reset to this
vhost device, by not just checking `k->get_vhost != NULL` (introduced by
commit 95e1019a4a9), but also that the vhost back-end is connected
(`hdev = k->get_vhost(); hdev != NULL && hdev->vhost_ops != NULL`).
I am not a native speaker but I think the commit message could be
rephrased to make it clear. This is a minor comment so feel free to
ignore it:
Thanks! I don’t mind restructuring, I just wanted to be verbose on
what’s happening. Honestly, I’m also not 100 % sure about my reasoning,
which is why I put a lot of indefinite fluff in there, i.e. purposefully
was not clear.
I wonder what exactly you find to be the problem with it, though? Too
much fluff, too much detail, weird sentence structure?
Before `virtio_reset()` is invoked, `virtio_set_status(vdev, 0)` for
vhost devices ends up setting `vhost_stared` to false thus resulting in
`vhost_reset_device()` being never reached during a reset.
It’s before vhost_reset_device(), not virtio_reset() (all of this is in
virtio_reset()). Also, the “`virtio_set_status()` for vhost devices”
sounds a bit strange and too detail-less to me; (A) it makes it sound a
bit like it’d be only called for vhost devices (easily solvable by
moving “for vhost devices” back after ”false”), and (B) I would like to
have the detail in there how vhost_started is cleared by the set_status
call, because it doesn’t seem obvious to me. Also, I find the “never
reached during a reset” strangely abstract; we can be very concrete
here, so why not be?
Maybe
When a vhost device is reset, we want `virtio_reset()` to call
`vhost_reset_device()`. However, `virtio_reset()` begins with
`virtio_set_status(vdev, 0)`, which, for vhost devices, will end up in
`vhost_dev_stop()`, clearing `vhost_started`. This makes the subsequent
`vhost_reset_device()` call unreachable.
`vhost_started` is not documented, however, from `vhost_dev_start()` and
`vhost_dev_stop()`, it seems indicating that there is a vhost back-end
and that the back-end is running and processing virtio requests.
Resetting should not rely on whether the vhost back-end is processing
virtio requests, instead, reset must happen if there is no processing
request. This is what `virtio_reset()` does, it first stops the backend
and then sends the reset.
I find this unclear on how vhost_dev_start()/stop() indicate what
vhost_started means (it’s because they set/clear it). I cannot stand
behind “reset must happen if there is no processing request”, because I
don’t know for sure. I just feel like it makes more sense this way,
hence me saying “probably”. I miss context to “it stops the backend”, I
would like to be clear that it’s through virtio_set_status(0) that the
back-end is stopped. I find it important to be concrete here because I
asked myself whether e could pull vhost_reset_device() up before
virtio_set_status(0), and this paragraph is intended to convey that that
would be wrong; I want to be clear that the concrete call order in that
function makes sense to me.
I also miss a connection to why this paragraph is focussing on
vhost_started now. Maybe I’m a bit slow at making connections, but in
this rephrased version, it is not 100 % clear that vhost_started is a
condition for vhost_reset_device() (unless one looks at the code
simultaneously), so its importance and why we need to know what it does
isn’t clear here. The very start of the original message however did
stress that the vhost_started condition seems wrong, so it’s clear why
it’s important at this point.
So maybe
Requiring `vhost_started` as a precondition for `vhost_reset_device()`
is also semantically questionable: While undocumented, it being
set/cleared by `vhost_dev_{start,stop}()` indicates that it shows
whether there is a vhost back-end, and whether that back-end is running
and processing virtio requests. The latter should not be a condition for
doing a reset, on the contrary: A reset should preferably happen when
the back-end is stopped. Thus, the order that we have in
`virtio_reset()` (stop the back-end via `virtio_set_status(vdev, 0)`,
then reset it) makes perfect sense, but checking `vhost_started` does not.
To trigger a reset, this commit drops the `vhost_started` condition and
checks that the device is running (introduced by 95e1019a4a9) but also
that the vhost back-end is connected so it is possible to send a reset
to the vhost device.
This seems incorrect: First, we decidedly no longer check that the
device is running, that was the point of the previous paragraph. Second,
no such check has been introduced by commit 95e1019a4a9. That commit
just introduced a check of `k->get_vhost != NULL`, i.e. whether this
could even theoretically be a vhost device.
So I’d just keep the final paragraph as-is from the original patch. It
isn’t much longer, just quotes more code.
Hanna
