zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.71k stars 6.54k forks source link

Corrupted timeout on poll for offloaded sockets #37472

Closed emillindq closed 3 years ago

emillindq commented 3 years ago

Describe the bug When pulling latest Zephyr for our project, I noticed that poll() suddenly went from blocking for 10ms to 30000ms. It seems like this PR is causing this timeout corruption with this addition: https://github.com/zephyrproject-rtos/zephyr/blob/ca5921845d4241ce9da9496a8e97d7dd6c1c4bae/subsys/net/lib/sockets/sockets.c#L1455-L1462

It seems like the intention is to convert ticks to ms, but the function sys_clock_timeout_end_calc returns at which tick in the future the timeout will happen. For the argument K_NO_WAIT it simply returns sys_clock_tick_get() with the definition

Return the current system tick count So instead of waiting 10ms, it waits for as long as the program has been running previously, plus 10ms, is my interpretation.

I'm not sure if this was the intention, but further down poll_timeout is used as time in ms as argument for k_sem_take(), at least in https://github.com/zephyrproject-rtos/zephyr/blob/ca5921845d4241ce9da9496a8e97d7dd6c1c4bae/drivers/modem/modem_socket.c#L307

Hence why it blocks for 30000ms.

Is my conclusion correct?

To Reproduce Steps to reproduce the behavior:

  1. Create offloaded socket
  2. Run the program for a while
  3. poll(..., ..., 10) and it will block for as long as it has run

Expected behavior Poll timeout should be intact when passed down

Impact Showstopper

Logs and console output N/A but it returns from poll() after 30s

Environment (please complete the following information):

emillindq commented 3 years ago

If changing to

        int poll_timeout;

        if (K_TIMEOUT_EQ(timeout, K_FOREVER)) {
            poll_timeout = SYS_FOREVER_MS;
        } else {
            poll_timeout = k_ticks_to_ms_floor32(
                timeout.ticks);
        }

it waits for 10ms again

Should I make a PR on this?

rlubos commented 3 years ago

Indeed, the k_timeout_t to ms conversion doesn't seem right in this case and was somehow overlooked in the original PR. I think that your proposal looks right, feel free to submit a PR!

emillindq commented 3 years ago

PR created, my unit tests pass with the fix :)