Re: [RFC 0/3] Vhost: fix FD entries cleanup

[Chenbo Xia <chenbox@nvidia.com>]

Hi David,

> On Feb 4, 2025, at 21:18, David Marchand <david.marchand@redhat.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello vhost maintainers,
> 
> On Tue, Dec 24, 2024 at 4:50 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> 
>> The vhost FD manager provides a way for the read/write
>> callbacks to request removal of their associated FD from
>> the epoll FD set. Problem is that it is missing a cleanup
>> callback, so the read/write callback requesting the removal
>> have to perform cleanups before the FD is removed from the
>> FD set. It includes closing the FD before it is removed
>> from the epoll FD set.
>> 
>> This series introduces a new cleanup callback which, if
>> implemented, is closed right after the FD is removed from
>> FD set.
>> 
>> Maxime Coquelin (3):
>>  vhost: add cleanup callback to FD entries
>>  vhost: fix vhost-user socket cleanup order
>>  vhost: improve VDUSE reconnect handler cleanup
>> 
>> lib/vhost/fd_man.c | 16 ++++++++++++----
>> lib/vhost/fd_man.h |  3 ++-
>> lib/vhost/socket.c | 46 ++++++++++++++++++++++++++--------------------
>> lib/vhost/vduse.c  | 16 +++++++++++-----
>> 4 files changed, 51 insertions(+), 30 deletions(-)
> 
> I tried this series, and it fixes the error log I reported.
> 
> On the other hand, I wonder if we could do something simpler.
> 
> The fd is only used by the registered handlers.
> If a handler reports that it does not want to watch this fd anymore,
> then there is no remaining user in the vhost library for this fd.
> 
> So my proposal would be to rename the "remove" flag as a "close" flag:
> 
> @@ -12,7 +12,7 @@ struct fdset;
> 
> #define MAX_FDS 1024
> 
> -typedef void (*fd_cb)(int fd, void *dat, int *remove);
> +typedef void (*fd_cb)(int fd, void *dat, int *close);
> 
> struct fdset *fdset_init(const char *name);
> 
> And defer closing to fd_man.
> Something like:
> 
> @@ -367,9 +367,9 @@ fdset_event_dispatch(void *arg)
>                        pthread_mutex_unlock(&pfdset->fd_mutex);
> 
>                        if (rcb && events[i].events & (EPOLLIN |
> EPOLLERR | EPOLLHUP))
> -                               rcb(fd, dat, &remove1);
> +                               rcb(fd, dat, &close1);
>                        if (wcb && events[i].events & (EPOLLOUT |
> EPOLLERR | EPOLLHUP))
> -                               wcb(fd, dat, &remove2);
> +                               wcb(fd, dat, &close2);
>                        pfdentry->busy = 0;
>                        /*
>                         * fdset_del needs to check busy flag.
> @@ -381,8 +381,10 @@ fdset_event_dispatch(void *arg)
>                         * fdentry not to be busy, so we can't call
>                         * fdset_del_locked().
>                         */
> -                       if (remove1 || remove2)
> +                       if (close1 || close2) {
>                                fdset_del(pfdset, fd);
> +                               close(fd);
> +                       }
>                }
> 
>                if (pfdset->destroy)
> 
> 
> And the only thing to move out of the socket and vduse handlers is the
> close(fd) call.
> 
> Like:
> 
> @@ -303,7 +303,7 @@ vhost_user_server_new_connection(int fd, void
> *dat, int *remove __rte_unused)
> }
> 
> static void
> -vhost_user_read_cb(int connfd, void *dat, int *remove)
> +vhost_user_read_cb(int connfd, void *dat, int *close)
> {
>        struct vhost_user_connection *conn = dat;
>        struct vhost_user_socket *vsocket = conn->vsocket;
> @@ -313,8 +313,7 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
>        if (ret < 0) {
>                struct virtio_net *dev = get_device(conn->vid);
> 
> -               close(connfd);
> -               *remove = 1;
> +               *close = 1;

I have one concern here is compared with this RFC, the proposal changed the timing
of close connfd，which means on QEMU side, cleaning up resources will happen later.

Currently I can’t think of issues could be introduced by this change (maybe you and
Maxime could remind me of something :)

Besides this, definitely this proposal is cleaner.

Thanks,
Chenbo 

> 
>                if (dev)
>                        vhost_destroy_device_notify(dev);
> 
> 
> Maxime, Chenbo, opinions?
> 
> 
> --
> David Marchand
>