Re: [PATCH v4 07/14] vfio-user: run vfio-user context

[Jag Raman]

> On Dec 20, 2021, at 3:29 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Fri, Dec 17, 2021 at 05:59:48PM +0000, Jag Raman wrote:
>> 
>> 
>>> On Dec 16, 2021, at 6:17 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> 
>>> On Wed, Dec 15, 2021 at 10:35:31AM -0500, Jagannathan Raman wrote:
>>>> @@ -114,6 +118,62 @@ static void vfu_object_set_device(Object *obj, const 
>>>> char *str, Error **errp)
>>>>    vfu_object_init_ctx(o, errp);
>>>> }
>>>> 
>>>> +static void vfu_object_ctx_run(void *opaque)
>>>> +{
>>>> +    VfuObject *o = opaque;
>>>> +    int ret = -1;
>>>> +
>>>> +    while (ret != 0) {
>>>> +        ret = vfu_run_ctx(o->vfu_ctx);
>>>> +        if (ret < 0) {
>>>> +            if (errno == EINTR) {
>>>> +                continue;
>>>> +            } else if (errno == ENOTCONN) {
>>>> +                qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
>>>> +                o->vfu_poll_fd = -1;
>>>> +                object_unparent(OBJECT(o));
>>>> +                break;
>>> 
>>> If nothing else logs a message then I think that should be done here so
>>> users know why their vfio-user server object disappeared.
>> 
>> Sure will do.
>> 
>> Do you prefer a trace, or a message to the console? Trace makes sense to me.
>> Presently, the client could unplug the vfio-user device which would trigger 
>> the
>> deletion of this object. This process could happen quietly.
> 
> If there is no way to differentiate graceful disconnect from unexpected
> disconnect then logging might be too noisy.

I think that’s what happens in the regular VFIO case also.
vfio_put_base_device() closes the FD used for ioctls.

> 
> Regarding the automatic deletion of the object, that might not be
> desirable for two reasons:
> 1. It prevents reconnection or another client connecting.
> 2. Management tools are in the dark about it.
> 
> For #2 there are monitor events that QEMU emits to notify management
> tools about state changes like disconnections.

This is very interesting. I suppose you’re referring to something like
‘BLOCK_JOB_COMPLETED’ event.

It’d be good to inform the management tools about disconnection. Not
used this before, will check it out to gather ideas on how to use it.

> 
> It's worth thinking about current and future use cases before baking in
> a policy like automatically deleting VfuObject on disconnect because
> it's inflexible and would require a QEMU update in the future to support
> a different policy.
> 
> One approach is to emit a disconnect event but leave the VfuObject in a
> disconnected state. The management tool can then restart or clean up,
> depending on its policy.
> 
> I'm not sure what's best because it depends on the use cases, but maybe
> you and others have ideas here.
> 
>>>> @@ -208,6 +284,8 @@ static void vfu_object_init(Object *obj)
>>>>                   TYPE_VFU_OBJECT, TYPE_REMOTE_MACHINE);
>>>>        return;
>>>>    }
>>>> +
>>>> +    o->vfu_poll_fd = -1;
>>>> }
>>> 
>>> This must call qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL)
>>> when o->vfu_poll_fd != -1 to avoid leaving a dangling fd handler
>>> callback registered.
>> 
>> This is during the init phase, and the FD handlers are not set. Do you mean
>> to add this at finalize?
>> 
>> I agree it’s good to explicitly add this at finalize. But vfu_destroy_ctx() 
>> should
>> trigger a ENOTCONN, which would do it anyway.
> 
> I'm not sure my comment makes sense since this is the init function, not
> finalize.
> 
> However, it's not clear to me that the o->vfu_poll_fd fd handler is
> unregistered from the event loop when VfuObject is finalized (e.g. by
> the object-del monitor command). You say vfu_destroy_ctx() triggers
> ENOTCONN, but the VfuObject is freed after finalize returns so when is
> the fd handler deregistered?

That is correct - will remove the FD handler in finalize also.

Thank you!
--
Jag

> 
> Stefan