Re: [PATCH v3 4/5] vhost: improve fdset initialization

[Maxime Coquelin]

Hi Chenbo,

On 4/26/24 09:40, Chenbo Xia wrote:

Hi Maxime,

On Apr 9, 2024, at 19:48, Maxime Coquelin <maxime.coque...@redhat.com> wrote:

External email: Use caution opening links or attachments


This patch heavily reworks fdset initialization:
- fdsets are now dynamically allocated by the FD manager
- the event dispatcher is now created by the FD manager
- struct fdset is now opaque to VDUSE and Vhost

Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
---
lib/vhost/fd_man.c | 177 ++++++++++++++++++++++++++++++++++++++++-----
lib/vhost/fd_man.h |  39 +---------
lib/vhost/socket.c |  24 ++----
lib/vhost/vduse.c  |  29 ++------
4 files changed, 177 insertions(+), 92 deletions(-)

diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c
index 0ae481b785..8b47c97d45 100644
--- a/lib/vhost/fd_man.c
+++ b/lib/vhost/fd_man.c
@@ -3,12 +3,16 @@
  */

#include <errno.h>
+#include <pthread.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

#include <rte_common.h>
#include <rte_log.h>
+#include <rte_malloc.h>
+#include <rte_string_fns.h>
+#include <rte_thread.h>

#include "fd_man.h"

@@ -19,6 +23,79 @@ RTE_LOG_REGISTER_SUFFIX(vhost_fdset_logtype, fdset, INFO);

#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)

+struct fdentry {
+       int fd;         /* -1 indicates this entry is empty */
+       fd_cb rcb;      /* callback when this fd is readable. */
+       fd_cb wcb;      /* callback when this fd is writeable.*/
+       void *dat;      /* fd context */
+       int busy;       /* whether this entry is being used in cb. */
+};
+
+struct fdset {
+       char name[RTE_THREAD_NAME_SIZE];
+       struct pollfd rwfds[MAX_FDS];
+       struct fdentry fd[MAX_FDS];
+       rte_thread_t tid;
+       pthread_mutex_t fd_mutex;
+       pthread_mutex_t fd_polling_mutex;
+       int num;        /* current fd number of this fdset */
+
+       union pipefds {
+               struct {
+                       int pipefd[2];
+               };
+               struct {
+                       int readfd;
+                       int writefd;
+               };
+       } u;
+
+       pthread_mutex_t sync_mutex;
+       pthread_cond_t sync_cond;
+       bool sync;
+       bool destroy;
+};
+
+static int fdset_add_no_sync(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb 
wcb, void *dat);
+static uint32_t fdset_event_dispatch(void *arg);
+
+#define MAX_FDSETS 8
+
+static struct fdset *fdsets[MAX_FDSETS];
+pthread_mutex_t fdsets_mutex = PTHREAD_MUTEX_INITIALIZER;

Static pthread_mutex_t ?

I think so, thanks for spotting it!
Will fix in next revision.

Maxime

Thanks,
Chenbo