k_queue_poll returns NULL with K_FOREVER

[carlescufi]

When running with #4350, we still see instances on Cortex-M4 (nRF52832) where k_queue_poll returns NULL, traced down to sys_slist_get() returning NULL, something that should never happen. We can reproduce relatively easily, although it seems to be a race condition.

[carlescufi]

@pfalcon @cvinayak @jhedberg FYI

[pfalcon]

@carlescufi : Can you provide more details about where it happens? Looking at k_queue_poll() code, it can be a situation that e.g. k_poll() returns that queue has elements, at which point context switches and some another thread gets this element out of it, and when context switches back, nothing is left. Any chance that you have multiple consumers?

[jhedberg]

@pfalcon @carlescufi I think multiple consumers may be a likely scenario, since the use case here involves a global net_buf pool which many places of the stack may be trying to allocate buffers from.

[carlescufi]

@jhedberg @pfalcon @cvinayak indeed there are multiple consumer threads for the net buf pool in this case, at least 3:

prio_recv_thread: allocates cmd complete packets
rx_thread: allocates events coming from the radio
tx_thread: allocates cmd complete

All are allocated in hci_raw from the same hci_rx_pool.

It would seem then that our assumption that net_buf_alloc(... K_FOREVER) will always return a valid buf is not valid when you call it from multiple threads with the same pool?

[carlescufi]

So the question for the kernel people, @andyross and @andrewboie would be: "can one call k_lifo_get(), i.e. k_queue_get() with K_FOREVER from multiple threads using a single queue and still expect to always get a pointer back that is not NULL, assuming no thread has explicitly cancelled the wait?"

[pfalcon]

Related patches: #777 , #1206

[andrewboie]

I think the non-NULL expectation makes the most sense to me, I can't think of advantages to having the API return NULL sometimes depending on how events line up

[cvinayak]

I agree with @andrewboie wrt non-NULL expectations. Lots of code depends on this expectation.

Looking at the non k_poll logic, _Swap returned a context safe non-NULL data ptr. I will be in favour of a similar k_poll irq locked return or k_poll returning with a guaranteed _current->base.swap_data something.

@Vudentz your code snippet with while loop that @carlescufi is testing changes API behavior, which should follow API deprecation rules (to maintain old behavior for next 2 releases). That said, introduction of k_queue_poll has already broken that rule on master, by returning NULLs.

[Vudentz]

@cvinayak The NULL return is a bug, Ive never said it wasn't. We could in theory do what mutex is doing:
https://github.com/zephyrproject-rtos/zephyr/blob/master/kernel/mutex.c#L155
Though the mutex code may also suffer from priority problems since the owner thread priority may be changed using k_thread_priority_set.