Hi David,
> On Feb 4, 2025, at 21:18, David Marchand <david.march...@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.coque...@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
>
