Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

epoll_wait with event_fd does not support EPOLLET mode #2462

Closed
huyuguang opened this issue Aug 31, 2017 · 9 comments
Closed

epoll_wait with event_fd does not support EPOLLET mode #2462

huyuguang opened this issue Aug 31, 2017 · 9 comments

Comments

@huyuguang
Copy link

huyuguang commented Aug 31, 2017

windows 10.0.15063, x64.
4.4.0-43-Microsoft #1-Microsoft Wed Dec 31 14:42:53 PST 2014 x86_64 x86_64 x86_64 GNU/Linux

I do not know if it is a bug of epoll_wait() or eventfd(), I wrote a demo code to prove it.
The code works fine in true linux, but not working in WSL.
If I add #define READ_TEST, the code works in WSL.
It seems that WSL does not implement EPOLLET correctly.

#include <stdlib.h>
#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <errno.h>
#include <unistd.h>
#include <assert.h>
#include <sys/epoll.h>
#include <sys/eventfd.h>
#include <thread>

int ep = -1;
int notify_fd = -1;
epoll_event event_list[100];
int nevents = (int)(sizeof(event_list)/sizeof(event_list[0]));

void run() {
    for (;;) {
        int events = epoll_wait(ep, event_list, nevents, -1);
        if (events == -1 && errno == EINTR)
            continue;
        if (events == -1 || events == 0) {
            printf("epoll_wait failed: %s\n", strerror(errno));
            abort();
        }

        assert(events == 1 && event_list[0].data.fd == notify_fd);
        printf("on_notify\n");

#ifdef READ_TEST
        // if we do not read here, the WSL does not working
        uint64_t ready = 0;
        ssize_t n = read(notify_fd, &ready, sizeof(ready));
        if (n != 8) {
            printf("read failed: %s\n", strerror(errno));
            abort();
        }  else {
            printf("ready: %ld\n", ready);
        }
#endif
    }
}

void init() {
    ep = epoll_create(100);
    if (ep == -1) {
        printf("epoll_create() failed: %s\n", strerror(errno));
        abort();
    }

    notify_fd = eventfd(0, 0);
    if (notify_fd == -1) {
        printf("eventfd failed: %s\n", strerror(errno));
        abort();
    }

    epoll_event ee;
    ee.events = EPOLLIN | EPOLLET;
    ee.data.fd = notify_fd;
    if (epoll_ctl(ep, EPOLL_CTL_ADD, notify_fd, &ee) == -1) {
        printf("epoll_ctl failed: %s\n", strerror(errno));
        abort();
    }
}

void notify() {
    static uint64_t inc = 1;

    if ((size_t)write(notify_fd, &inc, sizeof(uint64_t)) != sizeof(uint64_t)) {
        printf("write() to eventfd %d failed, %s\n", notify_fd,
            strerror(errno));
        abort();
    }
    printf("write() to eventfd %d\n", notify_fd);
}

int main()
{
    printf("hello from test_epoll!\n");

    init();

    std::thread t([]() {run(); });
    t.detach();

    printf("press any key to notify(q to exit)\n");
    for (;;) {
        int k = getchar();
        if (k == 'q') {
            exit(0);
        } else if (k != '\n') {
            notify();
        }
    }

    return 0;
}
@therealkenc
Copy link
Collaborator

therealkenc commented Aug 31, 2017

Okay, with effort I'm seeing it. Without #define READ_TEST on WSL:

ken@WinDev1612Sys:~/Devel/eventfd$ g++ -std=c++11 eventfd.cpp -lpthread
ken@WinDev1612Sys:~/Devel/eventfd$ ./a.out
hello from test_epoll!
press any key to notify(q to exit)
a
write() to eventfd 4
on_notify
a
write() to eventfd 4
a
write() to eventfd 4
^C

On WSL you only get the first printf("on_notify\n");. On Real Linux™ you get an on_notify every go-around. Which is to say, epoll_wait() waits forever after it fires the first time, unless the descriptor is drained with a read(). No point in an strace or I'd post it. There's no hard fails. The second epoll_wait() just never resumes.

@huyuguang
Copy link
Author

Yes. Thanks for your reply.
I found it when I debuged my nginx module in WSL. Nginx use the similar code to wake up main thread from worker thread (through nginx_notify()). I found I can only get first notify. So I wrote a test code to isolate the issue, that is the above code.

@therealkenc
Copy link
Collaborator

I gather from man epoll_wait(7) that any write (ie change) is considered a new "edge".

       The reason for this is that edge-triggered mode delivers events only when
       changes occur on the monitored file descriptor.

That said, NGINX doesn't seem to be following the "suggested way":

       The suggested way to use epoll as an edge-triggered (EPOLLET) interface 
       is as follows:
              i   with nonblocking file descriptors; and
              ii  by waiting for an event only after read(2) or write(2)
                  return EAGAIN.

The test case doesn't follow (ii).

Hopefully it's an easy fix. Good catch. Maybe it unblocks some unrelated stuff.

@huyuguang
Copy link
Author

huyuguang commented Aug 31, 2017

I guess the reason of Nginx does not "read untill return EAGAIN" is performance. The read(2) is a system call which has performance overhead. Since the "write(event_fd, 1)" is just use for wake up the epoll_wait, the following read(2) is useless.

In my first post, I said "I do not know if it is a bug of epoll_wait() or eventfd()". For normal fd, for example, socket, should keep read till got EAGAIN, but for eventfd, the read() is useless in most of scenario.

@therealkenc
Copy link
Collaborator

the read() is useless in most of scenario.

You're absolutely right. Events don't have to be "drained" like a real pipe. Arguably the whole point is not having to do the read. Thanks for sharing, because I learned something.

@sunilmut sunilmut added the bug label Sep 13, 2017
@sunilmut
Copy link
Member

@huyuguang - Thanks for the detailed report and explanation. @Brian-Perkins as submitted a fix for this and should make it's way to the Insider build soon.

@huyuguang
Copy link
Author

@Brian-Perkins - Very thanks, please comment here if it is fixed. My work heavily rely on WSL.

@sunilmut
Copy link
Member

This should be fixed in the Insider build 17017.

@huyuguang
Copy link
Author

Very thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants