-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Tuesday, September 20, 2022 8:18 AM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 2/2] dm: iothread: fix bug in iothread handler

On Mon, Sep 19, 2022 at 11:03:08PM +0800, Chen, Conghui wrote:

-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Monday, September 19, 2022 2:42 PM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 2/2] dm: iothread: fix bug in iothread handler

On Fri, Sep 16, 2022 at 04:35:29PM +0800, Conghui wrote:

Fix the bug in iothread handler, the event should be read out so that the
next epoll_wait not return directly as the fd can still readable.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/core/iothread.c | 11 +++++++++--
devicemodel/hw/pci/virtio/virtio.c | 1 +
devicemodel/include/iothread.h | 1 +
3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/devicemodel/core/iothread.c b/devicemodel/core/iothread.c
index 238ecb375..7fbce2b07 100644
--- a/devicemodel/core/iothread.c
+++ b/devicemodel/core/iothread.c
@@ -21,6 +21,7 @@


#define MEVENT_MAX 64
+#define MAX_EVENT_NUM 64
struct iothread_ctx {
pthread_t tid;
int epfd;
@@ -34,7 +35,8 @@ io_thread(void *arg)
{
struct epoll_event eventlist[MEVENT_MAX];
struct iothread_mevent *aevp;
- int i, n;
+ int i, n, status;
+ char buf[MAX_EVENT_NUM];

while(ioctx.started) {
n = epoll_wait(ioctx.epfd, eventlist, MEVENT_MAX, -1);
@@ -47,8 +49,13 @@ io_thread(void *arg)
}
for (i = 0; i < n; i++) {
aevp = eventlist[i].data.ptr;
- if (aevp && aevp->run)
+ if (aevp && aevp->run) {
+ /* dry the events in the fd */
+ do {
+ status = read(aevp->fd, buf,

sizeof(buf));

+ } while (status == MAX_EVENT_NUM);

I don't quite understand the point. There still has race to pop events
after this loop. We can not guarrante that. And what is the real impact
if the next epoll_wait return directly?

When kernel call event_signal(fd, 1), we can read 1 byte from aevp->fd. So,

we have to read the data out before we run the handler. Otherwise, in the
next loop, it will be treated as a new event_signal(fd, 1).
Then why do we need to read in loop? read on time is enough?

As kernel may invoke eventfd_signal (fd, 1) more than MAX_EVENT_NUM times, then iothread is scheduled.
So, need to make sure all data is read out.

(*aevp->run)(aevp->arg);
+ }
}
}

diff --git a/devicemodel/hw/pci/virtio/virtio.c

b/devicemodel/hw/pci/virtio/virtio.c

index 6b5646269..546ec6f31 100644
--- a/devicemodel/hw/pci/virtio/virtio.c
+++ b/devicemodel/hw/pci/virtio/virtio.c
@@ -104,6 +104,7 @@ virtio_set_iothread(struct virtio_base *base,
vq->viothrd.idx = idx;
vq->viothrd.iomvt.arg = &vq->viothrd;
vq->viothrd.iomvt.run = iothread_handler;
+ vq->viothrd.iomvt.fd = vq->viothrd.kick_fd;

if (!iothread_add(vq->viothrd.kick_fd, &vq-
viothrd.iomvt))
if (!virtio_register_ioeventfd(base, idx, true))
diff --git a/devicemodel/include/iothread.h

b/devicemodel/include/iothread.h

index 6a3cc7167..fb76e9baa 100644
--- a/devicemodel/include/iothread.h
+++ b/devicemodel/include/iothread.h
@@ -10,6 +10,7 @@
struct iothread_mevent {
void (*run)(void *);
void *arg;
+ int fd;
};
int iothread_add(int fd, struct iothread_mevent *aevt);
int iothread_del(int fd);
--
2.25.1