Re: [PATCH v4 1/2] vhost: dirty log should be per backend type

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 3/17/2024 8:20 PM, Jason Wang wrote:
> On Sat, Mar 16, 2024 at 2:33 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 3/14/2024 8:50 PM, Jason Wang wrote:
>>> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>> There could be a mix of both vhost-user and vhost-kernel clients
>>>> in the same QEMU process, where separate vhost loggers for the
>>>> specific vhost type have to be used. Make the vhost logger per
>>>> backend type, and have them properly reference counted.
>>> It's better to describe what's the advantage of doing this.
>> Yes, I can add that to the log. Although it's a niche use case, it was
>> actually a long standing limitation / bug that vhost-user and
>> vhost-kernel loggers can't co-exist per QEMU process, but today it's
>> just silent failure that may be ended up with. This bug fix removes that
>> implicit limitation in the code.
> Ok.
>
>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>
>>>> ---
>>>> v3->v4:
>>>>     - remove checking NULL return value from vhost_log_get
>>>>
>>>> v2->v3:
>>>>     - remove non-effective assertion that never be reached
>>>>     - do not return NULL from vhost_log_get()
>>>>     - add neccessary assertions to vhost_log_get()
>>>> ---
>>>>    hw/virtio/vhost.c | 45 +++++++++++++++++++++++++++++++++------------
>>>>    1 file changed, 33 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index 2c9ac79..612f4db 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -43,8 +43,8 @@
>>>>        do { } while (0)
>>>>    #endif
>>>>
>>>> -static struct vhost_log *vhost_log;
>>>> -static struct vhost_log *vhost_log_shm;
>>>> +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX];
>>>> +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX];
>>>>
>>>>    /* Memslots used by backends that support private memslots (without an fd). */
>>>>    static unsigned int used_memslots;
>>>> @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev *dev,
>>>>            r = -1;
>>>>        }
>>>>
>>>> +    if (r == 0) {
>>>> +        assert(dev->vhost_ops->backend_type == backend_type);
>>>> +    }
>>>> +
>>> Under which condition could we hit this?
>> Just in case some other function inadvertently corrupted this earlier,
>> we have to capture discrepancy in the first place... On the other hand,
>> it will be helpful for other vhost backend writers to diagnose day-one
>> bug in the code. I feel just code comment here will not be
>> sufficient/helpful.
> See below.
>
>>>    It seems not good to assert a local logic.
>> It seems to me quite a few local asserts are in the same file already,
>> vhost_save_backend_state,
> For example it has assert for
>
> assert(!dev->started);
>
> which is not the logic of the function itself but require
> vhost_dev_start() not to be called before.
>
> But it looks like this patch you assert the code just a few lines
> above the assert itself?
Yes, that was the intent - for e.g. xxx_ops may contain corrupted 
xxx_ops.backend_type already before coming to this 
vhost_set_backend_type() function. And we may capture this corrupted 
state by asserting the expected xxx_ops.backend_type (to be consistent 
with the backend_type passed in), which needs be done in the first place 
when this discrepancy is detected. In practice I think there should be 
no harm to add this assert, but this will add warranted guarantee to the 
current code.

Regards,
-Siwei

>
> dev->vhost_ops = &xxx_ops;
>
> ...
>
> assert(dev->vhost_ops->backend_type == backend_type)
>
> ?
>
> Thanks
>
>> vhost_load_backend_state,
>> vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local
>> assert a problem?
>>
>> Thanks,
>> -Siwei
>>
>>> Thanks
>>>