unix: switch uv_sleep() to nanosleep() #2552

bnoordhuis · 2019-11-26T09:46:36Z

Before this commit, uv_sleep() made two library calls: sleep() to sleep
for the requested number of seconds, and then an usleep() call to sleep
for the remaining milliseconds. Make a single nanosleep() call instead.

Receiving a signal will wake up prematurely from sleep() but then
re-enter sleep mode again in usleep(), which seems undesirable.

~~CI: https://ci.nodejs.org/job/libuv-test-commit/1634/~~
~~CI: https://ci.nodejs.org/job/libuv-test-commit/1636/~~
~~CI: https://ci.nodejs.org/job/libuv-test-commit/1650/~~
~~CI: https://ci.nodejs.org/job/libuv-test-commit/1652/~~
CI: https://ci.nodejs.org/job/libuv-test-commit/1653/

santigimeno

LGTM with a question

src/unix/core.c

cjihrig · 2019-11-28T14:35:30Z

On zOS, the CI is failing. I reran the CI a few times, but am still seeing the same failures:

not ok 210 - spawn_and_kill
# timeout
# Output from process `spawn_and_kill`:
# close_cb
# CEE5205S The signal SIGTERM was received.
not ok 211 - spawn_and_kill_with_std
# timeout
# Output from process `spawn_and_kill_with_std`:
# close_cb
# close_cb
# CEE5205S The signal SIGTERM was received.

cjihrig · 2019-12-03T16:04:14Z

I'm a bit confused. I found this article, which states:

The cond_timed_wait service is similar to the POSIX function nanosleep(). (Refer to
the POSIX standard for a description of nanosleep().) If you need the nanosleep()
function, you can use cond_timed_wait to implement your own version.

Does that mean nanosleep() is implemented, but does not work properly on zOS? Are the CI failures not actually related to this PR?

cc: @libuv/zos

bnoordhuis · 2019-12-03T16:34:16Z

@libuv/zos For context, there might be a bug in libuv-on-zos's nanosleep() emulation here:

libuv/src/unix/os390-syscalls.c

Lines 346 to 374 in 5500253

    
           int nanosleep(const struct timespec* req, struct timespec* rem) { 
        
             unsigned nano; 
        
             unsigned seconds; 
        
             unsigned events; 
        
             unsigned secrem; 
        
             unsigned nanorem; 
        
             int rv; 
        
             int rc; 
        
             int rsn; 
        
             nano = (int)req->tv_nsec; 
        
             seconds = req->tv_sec; 
        
             events = CW_CONDVAR; 
        
           #if defined(_LP64) 
        
             BPX4CTW(&seconds, &nano, &events, &secrem, &nanorem, &rv, &rc, &rsn); 
        
           #else 
        
             BPX1CTW(&seconds, &nano, &events, &secrem, &nanorem, &rv, &rc, &rsn); 
        
           #endif 
        
             assert(rv == -1 && errno == EAGAIN); 
        
             if(rem != NULL) { 
        
               rem->tv_nsec = nanorem; 
        
               rem->tv_sec = secrem; 
        
             } 
        
             return 0; 
        
           }

Notably, this PR starts using the rem out parameter, which was unused before.

richardlau · 2019-12-03T17:19:48Z

@libuv/zos For context, there might be a bug in libuv-on-zos's nanosleep() emulation here:

libuv/src/unix/os390-syscalls.c

Lines 346 to 374 in 5500253

int nanosleep(const struct timespec* req, struct timespec* rem) {

unsigned nano;

unsigned seconds;

unsigned events;

unsigned secrem;

unsigned nanorem;

int rv;

int rc;

int rsn;

nano = (int)req->tv_nsec;

seconds = req->tv_sec;

events = CW_CONDVAR;

#if defined(_LP64)

BPX4CTW(&seconds, &nano, &events, &secrem, &nanorem, &rv, &rc, &rsn);

#else

BPX1CTW(&seconds, &nano, &events, &secrem, &nanorem, &rv, &rc, &rsn);

#endif

assert(rv == -1 && errno == EAGAIN);

if(rem != NULL) {

rem->tv_nsec = nanorem;

rem->tv_sec = secrem;

}

return 0;

}

Notably, this PR starts using the rem out parameter, which was unused before.

According to the documentation that @cjihrig linked to BPX4CTW/BPX1CTW returns EAGAIN/EINTR/EINVAL in the return code parameter (rc in the code above) and makes no mention of errno. Although if errno is anything other than EAGAIN the assert on L366 would trigger.

cc @ccw-1 and @zsw007 since there have been changes to this function that haven't been upstreamed here.

Before this commit, uv_sleep() made two library calls: sleep() to sleep for the requested number of seconds, and then an usleep() call to sleep for the remaining milliseconds. Make a single nanosleep() call instead. Receiving a signal will wake up prematurely from sleep() but then re-enter sleep mode again in usleep(), which seems undesirable.

Reception of a signal makes nanosleep() return prematurely. Restart the system call when that happens.

* Remove an assert() that wasn't actually being tested because the CI builds libuv with `-DNDEBUG` on that platform. * Fix the emulation to update `errno` correctly. * Fix the emulation to update the `rem` out parameter correctly.

bnoordhuis · 2019-12-03T18:59:12Z

Although if errno is anything other than EAGAIN the assert on L366 would trigger.

It looks like libuv is built with -DNDEBUG on z/os and hence the macro isn't compiled.

The nanosleep() emulation looks buggy to me after reading the docs on BPX1CTW/BPX4CTW. I've pushed a commit that hopefully fixes it, PTAL.

CI: https://ci.nodejs.org/job/libuv-test-commit/1650/ - edit: still failing...

richardlau · 2019-12-03T19:17:44Z

Looks like it still failed... reading the failing tests (spawn_and_kill and spawn_and_kill_with_std) it looks like the tests send a signal to interrupt so perhaps

libuv/src/unix/os390-syscalls.c

Line 358 in 5500253

events = CW_CONDVAR;

should be

 events = CW_CONDVAR | CW_INTRPT;

based on the documentation:

If you do not include the CW_INTRPT event when you use cond_timed_wait, some services that are used by other threads or processes cannot cause the waiting thread to resume processing. In particular, the following services do not cause an event notification unless CW_INTRPT is specified in the event list:

kill

pthread_cancel

pthread_kill

pthread_quiesce

bnoordhuis · 2019-12-03T19:21:41Z

Worth a try, thanks! New CI: https://ci.nodejs.org/job/libuv-test-commit/1652/

edit: of course it fails to build due to a Undeclared identifier CW_INTRPT error... CW_CONDVAR is defined on line 30 in that file but I don't know what the value for CW_INTRPT should be.

richardlau · 2019-12-03T19:27:47Z

Looks like we'll need to define CW_INTRPT. Based on https://github.com/ibmruntimes/node/blob/0b9900ffc6897254492cd1883b9ed987741d55bf/deps/zoslib/include/zos.h#L99 add

#define CW_INTRPT 1

just before

libuv/src/unix/os390-syscalls.c

Line 31 in 5500253

#define CW_CONDVAR 32

?

Edit: found a source for what the values should be: https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.4.0/com.ibm.zos.v2r4.bpxb100/ycw.htm

bnoordhuis · 2019-12-03T19:32:36Z

Done, thanks! New CI: https://ci.nodejs.org/job/libuv-test-commit/1653/ - edit: green!

richardlau

z/OS changes LGTM.

Before this commit, uv_sleep() made two library calls: sleep() to sleep for the requested number of seconds, and then an usleep() call to sleep for the remaining milliseconds. Make a single nanosleep() call instead. Receiving a signal will wake up prematurely from sleep() but then re-enter sleep mode again in usleep(), which seems undesirable. PR-URL: #2552 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Reception of a signal makes nanosleep() return prematurely. Restart the system call when that happens. PR-URL: #2552 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

* Remove an assert() that wasn't actually being tested because the CI builds libuv with `-DNDEBUG` on that platform. * Fix the emulation to update `errno` correctly. * Fix the emulation to update the `rem` out parameter correctly. PR-URL: #2552 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

cjihrig · 2019-12-04T00:19:19Z

Landed in 5500253...635e0ce. Thanks!

santigimeno approved these changes Nov 26, 2019

View reviewed changes

src/unix/core.c Outdated Show resolved Hide resolved

saghul approved these changes Nov 26, 2019

View reviewed changes

richardlau approved these changes Nov 26, 2019

View reviewed changes

cjihrig approved these changes Nov 26, 2019

View reviewed changes

src/unix/core.c Outdated Show resolved Hide resolved

bnoordhuis force-pushed the nanosleep branch from 14ca743 to e2167d3 Compare November 28, 2019 11:23

santigimeno approved these changes Nov 28, 2019

View reviewed changes

bnoordhuis added 3 commits December 3, 2019 19:56

unix: retry on EINTR in uv_sleep()

ff57d00

Reception of a signal makes nanosleep() return prematurely. Restart the system call when that happens.

zos: fix nanosleep() emulation

d360380

* Remove an assert() that wasn't actually being tested because the CI builds libuv with `-DNDEBUG` on that platform. * Fix the emulation to update `errno` correctly. * Fix the emulation to update the `rem` out parameter correctly.

bnoordhuis force-pushed the nanosleep branch from e2167d3 to d360380 Compare December 3, 2019 18:58

richardlau approved these changes Dec 3, 2019

View reviewed changes

fixup! or in CW_INTRPT

48e9d59

fixup! define CW_INTRPT

7a05fbb

richardlau approved these changes Dec 3, 2019

View reviewed changes

cjihrig closed this Dec 4, 2019

richardlau mentioned this pull request Mar 17, 2020

build: fix z/OS cmake build #2737

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix: switch uv_sleep() to nanosleep() #2552

unix: switch uv_sleep() to nanosleep() #2552

bnoordhuis commented Nov 26, 2019 •

edited

Loading

santigimeno left a comment

cjihrig commented Nov 28, 2019

cjihrig commented Dec 3, 2019

bnoordhuis commented Dec 3, 2019

richardlau commented Dec 3, 2019 •

edited

Loading

bnoordhuis commented Dec 3, 2019 •

edited

Loading

richardlau commented Dec 3, 2019

bnoordhuis commented Dec 3, 2019 •

edited

Loading

richardlau commented Dec 3, 2019 •

edited

Loading

bnoordhuis commented Dec 3, 2019 •

edited

Loading

richardlau left a comment

cjihrig commented Dec 4, 2019

unix: switch uv_sleep() to nanosleep() #2552

unix: switch uv_sleep() to nanosleep() #2552

Conversation

bnoordhuis commented Nov 26, 2019 • edited Loading

santigimeno left a comment

Choose a reason for hiding this comment

cjihrig commented Nov 28, 2019

cjihrig commented Dec 3, 2019

bnoordhuis commented Dec 3, 2019

richardlau commented Dec 3, 2019 • edited Loading

bnoordhuis commented Dec 3, 2019 • edited Loading

richardlau commented Dec 3, 2019

bnoordhuis commented Dec 3, 2019 • edited Loading

richardlau commented Dec 3, 2019 • edited Loading

bnoordhuis commented Dec 3, 2019 • edited Loading

richardlau left a comment

Choose a reason for hiding this comment

cjihrig commented Dec 4, 2019

bnoordhuis commented Nov 26, 2019 •

edited

Loading

richardlau commented Dec 3, 2019 •

edited

Loading

bnoordhuis commented Dec 3, 2019 •

edited

Loading

bnoordhuis commented Dec 3, 2019 •

edited

Loading

richardlau commented Dec 3, 2019 •

edited

Loading

bnoordhuis commented Dec 3, 2019 •

edited

Loading