kernel: pipe: if timeout is not K_NO_WAIT and >= min_xfer bytes transferred, then return immediately #24486

cfriedt · 2020-04-19T00:27:16Z

If timeout != K_NO_WAIT, then return immediately when not all bytes_to_read / bytes_to_write have been transfered, but >= min_xfer have been transferred.

Fixes #24485

zephyrbot · 2020-04-19T00:57:47Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

andyross

This looks correct to me. My one complaint is that's special casing K_FOREVER when, as I see it, we should be treating the timeout symmetrically. That is, our behavior is wrong for non-zero finite timeouts too, and we should fix that at the same time. I'm pretty sure it's just the change to the conditional.

And we should probably add a test of this behavior too..

kernel/pipes.c

andyross

Looks great to me, thanks.

cfriedt · 2020-04-23T11:41:58Z

@andyross - any idea what is up with Shippable? It looks like some invalid memory accesses are happening, particularly in case 4.

I'm also encountering something similar with my implementation of socketpair(2) (which does not include this change).

AFAICT, I'm just using the k_pipe API as prescribed in my test cases here, as well as in the socketpair(2) implementation.

Does it seem like something else is up with k_pipe to you?

cfriedt · 2020-04-23T14:27:48Z

I've created another issue (#24642) that demonstrates the issue on master.

cfriedt · 2020-04-23T19:09:26Z

I've created another issue (#24642) that demonstrates the issue on master.

Learned about k_object_alloc() in #24642 which explains a lot. Latest revision should work fine with BOARD=qemu_x86_64 as well.

In short - kernel objects cannot be stack allocated (edit: in userspace threads)!

andyross · 2020-04-23T19:14:07Z

In short - kernel objects cannot be stack allocated!

They can, but not in userspace threads. Also there may be a mode coming soon for cache-incoherent multiprocessors where the local stack will be cached where IPC data will be placed in uncached regions by default. But yeah, it's virtually always best to declare these things statically using the existing initializer macros.

cfriedt · 2020-04-23T19:16:33Z

In short - kernel objects cannot be stack allocated!

They can, but not in userspace threads. Also there may be a mode coming soon for cache-incoherent multiprocessors where the local stack will be cached where IPC data will be placed in uncached regions by default. But yeah, it's virtually always best to declare these things statically using the existing initializer macros.

Ah, I see.

carlescufi · 2020-04-24T15:49:40Z

@nashif this is timing out, should we merge directly?

andrewboie · 2020-04-27T18:33:22Z

close/reopen to kick CI again

nashif · 2020-04-27T20:01:01Z

@cfriedt the timeout is related to your change, please see why

tests/benchmarks/app_kernel is hanging with native_posix...

sanitycheck -p native_posix -T tests/benchmarks/app_kernel -vv

nashif · 2020-04-27T20:04:33Z

or west build -b native_posix tests/benchmarks/app_kernel -t run

carlescufi · 2020-04-27T20:07:31Z

or west build -b native_posix tests/benchmarks/app_kernel -t run

Same here, I just checked and the test hangs forever as well when using this PR vs master

carlescufi · 2020-04-27T20:09:19Z

On the other hand, the test you modified passes correctly:

$ west build -b native_posix tests/kernel/pipe/pipe -t run
-- west build: running target run
[0/1] cd /home/carles/src/zephyr/zephyr/build && /home/carles/src/zephyr/zephyr/build/zephyr/zephyr.exe
*** Booting Zephyr OS build zephyr-v2.2.0-1792-g0014aeac3212  ***
Running test suite test_pipe
===================================================================
starting test - test_pipe_on_single_elements
PASS - test_pipe_on_single_elements
===================================================================
starting test - test_pipe_on_multiple_elements
PASS - test_pipe_on_multiple_elements
===================================================================
starting test - test_pipe_forever_wait
PASS - test_pipe_forever_wait
===================================================================
starting test - test_pipe_timeout
PASS - test_pipe_timeout
===================================================================
starting test - test_pipe_get_on_empty_pipe
PASS - test_pipe_get_on_empty_pipe
===================================================================
starting test - test_pipe_forever_timeout
PASS - test_pipe_forever_timeout
===================================================================
starting test - test_pipe_get_timeout
PASS - test_pipe_get_timeout
===================================================================
starting test - test_pipe_get_invalid_size
PASS - test_pipe_get_invalid_size
===================================================================
starting test - test_pipe_get_min_xfer
PASS - test_pipe_get_min_xfer
===================================================================
starting test - test_pipe_put_min_xfer
PASS - test_pipe_put_min_xfer
===================================================================
Test suite test_pipe succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL

andyross · 2020-04-27T20:11:13Z

To be clear: we've had a nontrivial number of situations where tests have been written to demand what seems like genuinely buggy behavior. Ping me if you get stuck. I promise I'm personally convinced this behavior is what we want.

carlescufi · 2020-04-27T20:12:08Z

@cfriedt it seems stuck in k_pipe_put():

(gdb) info threads
  Id   Target Id                                   Frame
* 1    Thread 0xf7fcea40 (LWP 130276) "zephyr.exe" 0xf7fd2b49 in __kernel_vsyscall ()
  3    Thread 0xf72ffb40 (LWP 130281) "zephyr.exe" 0x5655a9bd in k_pipe_put (data=<optimized out>, timeout=..., min_xfer=0, bytes_written=0xf72ff218, bytes_to_write=8, pipe=0x565677c0 <PIPE_NOBUFF>)
    at zephyr/include/generated/syscalls/kernel.h:939
  4    Thread 0xf6afeb40 (LWP 130282) "zephyr.exe" 0xf7fd2b49 in __kernel_vsyscall ()
  5    Thread 0xf60ffb40 (LWP 130283) "zephyr.exe" 0xf7fd2b49 in __kernel_vsyscall ()
(gdb) thread 3
[Switching to thread 3 (Thread 0xf72ffb40 (LWP 130281))]
#0  0x5655a9bd in k_pipe_put (data=<optimized out>, timeout=..., min_xfer=0, bytes_written=0xf72ff218, bytes_to_write=8, pipe=0x565677c0 <PIPE_NOBUFF>) at zephyr/include/generated/syscalls/kernel.h:939
939             return z_impl_k_pipe_put(pipe, data, bytes_to_write, bytes_written, min_xfer, timeout);
(gdb) bt
#0  0x5655a9bd in k_pipe_put (data=<optimized out>, timeout=..., min_xfer=0, bytes_written=0xf72ff218, bytes_to_write=8, pipe=0x565677c0 <PIPE_NOBUFF>) at zephyr/include/generated/syscalls/kernel.h:939
#1  pipeput (pipe=0x565677c0 <PIPE_NOBUFF>, option=_1_TO_N, size=8, count=256, time=0xf72ff290) at /home/carles/src/zephyr/zephyr/tests/benchmarks/app_kernel/src/pipe_b.c:219
#2  0x5655ad6c in pipe_test () at /home/carles/src/zephyr/zephyr/tests/benchmarks/app_kernel/src/pipe_b.c:169
#3  0x5655a38c in zephyr_app_main () at /home/carles/src/zephyr/zephyr/tests/benchmarks/app_kernel/src/master.c:140
#4  0x5655e34b in bg_thread_main (unused1=0x0, unused2=0x0, unused3=0x0) at /home/carles/src/zephyr/zephyr/kernel/init.c:261
#5  0x5655ba5e in z_thread_entry (entry=0x5655e303 <bg_thread_main>, p1=0x0, p2=0x0, p3=0x0) at /home/carles/src/zephyr/zephyr/lib/os/thread_entry.c:29
#6  0x5655c47f in posix_thread_starter (arg=0x0) at /home/carles/src/zephyr/zephyr/arch/posix/core/posix_core.c:305
#7  0xf7e90695 in start_thread (arg=0xf72ffb40) at pthread_create.c:479
#8  0xf7da2a5a in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:108

carlescufi · 2020-04-27T20:13:40Z

To be clear: we've had a nontrivial number of situations where tests have been written to demand what seems like genuinely buggy behavior. Ping me if you get stuck. I promise I'm personally convinced this behavior is what we want.

It might well be that this PR needs to come with a change to tests/benchmarks/app_kernel then.

cfriedt · 2020-04-28T01:42:40Z

Interesting - thanks for looking at this test case everyone. From the stack trace @carlescufi provided, it looks as though min_xfer = 0, which is a case I'd not considered.

If K_FOREVER is specified as the timeout, and min_xfer = 0, then the current patch will return if zero bytes have been written. However, for a blocking write, normally at least 1 byte must be transferred in order to justify returning early.

I think the fix is to just add the extra condition && min_xfer > 0 for both k_pipe_put() and k_pipe_get().

Let me update the PR.

If timeout != K_NO_WAIT, then return immediately when not all bytes_to_read or bytes_to_write have been transfered, but >= min_xfer have been transferred. Fixes #24485 Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>

carlescufi · 2020-04-28T14:14:18Z

~~@cfriedt still stuck: https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/70528/2/console~~

Ignore this comment, I had not refreshed the page. Merging now.

cfriedt requested review from andrewboie and andyross as code owners April 19, 2020 00:27

zephyrbot added the area: Kernel label Apr 19, 2020

cfriedt requested a review from nashif as a code owner April 19, 2020 00:55

zephyrbot added the area: Tests Issues related to a particular existing or missing test label Apr 19, 2020

This was referenced Apr 19, 2020

kernel: pipe: should return if >= min_xfer bytes transferred and timeout is K_FOREVER #24485

Closed

kernel: fix runtime initialization of k_pipe object #24526

Merged

andyross requested changes Apr 21, 2020

View reviewed changes

kernel/pipes.c Outdated Show resolved Hide resolved

cfriedt changed the title ~~kernel: pipe: if timeout is K_FOREVER and >= min_xfer bytes transferred, then return immediately~~ kernel: pipe: if timeout is not K_NO_WAIT and >= min_xfer bytes transferred, then return immediately Apr 21, 2020

andyross approved these changes Apr 21, 2020

View reviewed changes

cfriedt mentioned this pull request Apr 23, 2020

kernel: pipe: simple test fails for pipe write / read of 3 bytes #24642

Closed

andrewboie approved these changes Apr 27, 2020

View reviewed changes

andrewboie closed this Apr 27, 2020

andrewboie reopened this Apr 27, 2020

carlescufi merged commit d650f4b into zephyrproject-rtos:master Apr 28, 2020

cfriedt deleted the issue/24485/k-pipe-get-does-not-respect-min-xfer branch April 28, 2020 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: pipe: if timeout is not K_NO_WAIT and >= min_xfer bytes transferred, then return immediately #24486

kernel: pipe: if timeout is not K_NO_WAIT and >= min_xfer bytes transferred, then return immediately #24486

cfriedt commented Apr 19, 2020 •

edited

Loading

zephyrbot commented Apr 19, 2020 •

edited

Loading

andyross left a comment

andyross left a comment

cfriedt commented Apr 23, 2020

cfriedt commented Apr 23, 2020

cfriedt commented Apr 23, 2020 •

edited

Loading

andyross commented Apr 23, 2020

cfriedt commented Apr 23, 2020

carlescufi commented Apr 24, 2020

andrewboie commented Apr 27, 2020

nashif commented Apr 27, 2020

nashif commented Apr 27, 2020

carlescufi commented Apr 27, 2020

carlescufi commented Apr 27, 2020

andyross commented Apr 27, 2020

carlescufi commented Apr 27, 2020

carlescufi commented Apr 27, 2020

cfriedt commented Apr 28, 2020 •

edited

Loading

carlescufi commented Apr 28, 2020 •

edited

Loading

kernel: pipe: if timeout is not K_NO_WAIT and >= min_xfer bytes transferred, then return immediately #24486

kernel: pipe: if timeout is not K_NO_WAIT and >= min_xfer bytes transferred, then return immediately #24486

Conversation

cfriedt commented Apr 19, 2020 • edited Loading

zephyrbot commented Apr 19, 2020 • edited Loading

andyross left a comment

Choose a reason for hiding this comment

andyross left a comment

Choose a reason for hiding this comment

cfriedt commented Apr 23, 2020

cfriedt commented Apr 23, 2020

cfriedt commented Apr 23, 2020 • edited Loading

andyross commented Apr 23, 2020

cfriedt commented Apr 23, 2020

carlescufi commented Apr 24, 2020

andrewboie commented Apr 27, 2020

nashif commented Apr 27, 2020

nashif commented Apr 27, 2020

carlescufi commented Apr 27, 2020

carlescufi commented Apr 27, 2020

andyross commented Apr 27, 2020

carlescufi commented Apr 27, 2020

carlescufi commented Apr 27, 2020

cfriedt commented Apr 28, 2020 • edited Loading

carlescufi commented Apr 28, 2020 • edited Loading

cfriedt commented Apr 19, 2020 •

edited

Loading

zephyrbot commented Apr 19, 2020 •

edited

Loading

cfriedt commented Apr 23, 2020 •

edited

Loading

cfriedt commented Apr 28, 2020 •

edited

Loading

carlescufi commented Apr 28, 2020 •

edited

Loading