Request for review: 7166048: remove the embedded epoll data structure

[Sean Chou]

Hi Alan,

    The new patch is at:
http://cr.openjdk.java.net/~zhouyx/7166048/webrev.03/  . There are some
spaces difference between the old and new file. Please take a look again.

On Fri, May 4, 2012 at 3:59 PM, Alan Bateman <Alan.Bateman at oracle.com>wrote:

> On 04/05/2012 08:35, Sean Chou wrote:
>
>> Hi all,
>>
>>    I found the src/solaris/native/sun/nio/ch/**EPollArrayWrapper.c
>> embedded the epoll data structure epoll_data and epoll_event . It is
>> duplicated with the definition in sys/epoll.h . This redundancy would cause
>> failure if the underlying system has different epoll data structure (eg.
>> different alignment) .
>>
>>    I reported 7166048 for it and made a patch:
>> http://cr.openjdk.java.net/~**zhouyx/7166048/webrev.00/<http://cr.openjdk.java.net/~zhouyx/7166048/webrev.00/><
>> http://cr.openjdk.java.net/%**7Ezhouyx/7166048/webrev.00/<http://cr.openjdk.java.net/%7Ezhouyx/7166048/webrev.00/>>
>>  .
>>
>>
>>  As background, when the epoll Selector was added then the JDK had to be
> build on distributions that were still 2.4 based and didn't have epoll.h.
> We had meant to go back and clear this up in JDK7 but didn't get around to
> it. So thanks for taking this on now. I looked at your patch but it doesn't
> seem to be complete as it doesn't change the usages to call the epoll
> functions directly. Attached is the patch that I had for cleaning this up,
> it should still be current because this code has not changed. The only
> thing I notice is that it doesn't remove the include of dlfcn.h, that
> shouldn't be needed now.
>
> On the RFE that you submitted, I've moved this to the right place as:
>
> 7166048: (se) EPollArrayWrapper.c no longer needs to define epoll data
> structures
>
> -Alan.
>
>
>
> diff --git a/src/solaris/native/sun/nio/**ch/EPollArrayWrapper.c
> b/src/solaris/native/sun/nio/**ch/EPollArrayWrapper.c
> --- a/src/solaris/native/sun/nio/**ch/EPollArrayWrapper.c
> +++ b/src/solaris/native/sun/nio/**ch/EPollArrayWrapper.c
> @@ -34,55 +34,13 @@
>  #include <unistd.h>
>  #include <sys/resource.h>
>  #include <sys/time.h>
> -
> -#ifdef  __cplusplus
> -extern "C" {
> -#endif
> -
> -/* epoll_wait(2) man page */
> -
> -typedef union epoll_data {
> -    void *ptr;
> -    int fd;
> -    __uint32_t u32;
> -    __uint64_t u64;
> -} epoll_data_t;
> -
> -
> -/* x86-64 has same alignment as 32-bit */
> -#ifdef __x86_64__
> -#define EPOLL_PACKED __attribute__((packed))
> -#else
> -#define EPOLL_PACKED
> -#endif
> -
> -struct epoll_event {
> -    __uint32_t events;  /* Epoll events */
> -    epoll_data_t data;  /* User data variable */
> -} EPOLL_PACKED;
> -
> -#ifdef  __cplusplus
> -}
> -#endif
> +#include <sys/epoll.h>
>
>  #define RESTARTABLE(_cmd, _result) do { \
>   do { \
>     _result = _cmd; \
>   } while((_result == -1) && (errno == EINTR)); \
>  } while(0)
> -
> -/*
> - * epoll event notification is new in 2.6 kernel. As the offical build
> - * platform for the JDK is on a 2.4-based distribution then we must
> - * obtain the addresses of the epoll functions dynamically.
> - */
> -typedef int (*epoll_create_t)(int size);
> -typedef int (*epoll_ctl_t)   (int epfd, int op, int fd, struct
> epoll_event *event);
> -typedef int (*epoll_wait_t)  (int epfd, struct epoll_event *events, int
> maxevents, int timeout);
> -
> -static epoll_create_t epoll_create_func;
> -static epoll_ctl_t    epoll_ctl_func;
> -static epoll_wait_t   epoll_wait_func;
>
>  static int
>  iepoll(int epfd, struct epoll_event *events, int numfds, jlong timeout)
> @@ -96,7 +54,7 @@ iepoll(int epfd, struct epoll_event *eve
>     start = t.tv_sec * 1000 + t.tv_usec / 1000;
>
>     for (;;) {
> -        int res = (*epoll_wait_func)(epfd, events, numfds, timeout);
> +        int res = epoll_wait(epfd, events, numfds, timeout);
>         if (res < 0 && errno == EINTR) {
>             if (remaining >= 0) {
>                 gettimeofday(&t, NULL);
> @@ -117,14 +75,6 @@ JNIEXPORT void JNICALL
>  JNIEXPORT void JNICALL
>  Java_sun_nio_ch_**EPollArrayWrapper_init(JNIEnv *env, jclass this)
>  {
> -    epoll_create_func = (epoll_create_t) dlsym(RTLD_DEFAULT,
> "epoll_create");
> -    epoll_ctl_func    = (epoll_ctl_t)    dlsym(RTLD_DEFAULT, "epoll_ctl");
> -    epoll_wait_func   = (epoll_wait_t)   dlsym(RTLD_DEFAULT,
> "epoll_wait");
> -
> -    if ((epoll_create_func == NULL) || (epoll_ctl_func == NULL) ||
> -        (epoll_wait_func == NULL)) {
> -        JNU_ThrowInternalError(env, "unable to get address of epoll
> functions, pre-2.6 kernel?");
> -    }
>  }
>
>  JNIEXPORT jint JNICALL
> @@ -134,7 +84,7 @@ Java_sun_nio_ch_**EPollArrayWrapper_epollC
>      * epoll_create expects a size as a hint to the kernel about how to
>      * dimension internal structures. We can't predict the size in advance.
>      */
> -    int epfd = (*epoll_create_func)(256);
> +    int epfd = epoll_create(256);
>     if (epfd < 0) {
>        JNU_**ThrowIOExceptionWithLastError(**env, "epoll_create failed");
>     }
> @@ -173,7 +123,7 @@ Java_sun_nio_ch_**EPollArrayWrapper_epollC
>     event.events = events;
>     event.data.fd = fd;
>
> -    RESTARTABLE((*epoll_ctl_func)(**epfd, (int)opcode, (int)fd, &event),
> res);
> +    RESTARTABLE(epoll_ctl(epfd, (int)opcode, (int)fd, &event), res);
>
>     /*
>      * A channel may be registered with several Selectors. When each
> Selector
> @@ -199,7 +149,7 @@ Java_sun_nio_ch_**EPollArrayWrapper_epollW
>     int res;
>
>     if (timeout <= 0) {           /* Indefinite or no wait */
> -        RESTARTABLE((*epoll_wait_func)**(epfd, events, numfds, timeout),
> res);
> +        RESTARTABLE(epoll_wait(epfd, events, numfds, timeout), res);
>     } else {                      /* Bounded wait; bounded restarts */
>         res = iepoll(epfd, events, numfds, timeout);
>     }
>
>


-- 
Best Regards,
Sean Chou
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20120507/00f43349/attachment.html