net: sockets: Add locking to receive callback #35466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pfalcon I did testing with |
jukkar
force-pushed
the
bug-34964-socket-locking-regression
branch
from
cc5a5f5 to
09ff600
Compare
rlubos
reviewed
May 20, 2021
Fix a regression when application is waiting data but does not notice that because socket layer is not woken up. This could happen because application was waiting condition variable but the signal to wake the condvar came before the wait started. Normally if there is constant flow of incoming data to the socket, the signal would be given later. But if the peer is waiting that Zephyr replies, there might be a timeout at peer. The solution is to add locking in socket receive callback so that we only signal the condition variable after we have made sure that the condition variable is actually waiting the data. Fixes zephyrproject-rtos#34964 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
If we are waiting all the data i.e., the MSG_WAITALL flag is set, then if we have not yet received all the data at the end of the receive loop. We must use the condition variable to get the signal when the data is ready to be received. Otherwise the receive loop will not release the socket lock and receive_cb will not be able to indicate that data is received. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
The k_fifo_ prefix is meant for kernel API functions, and not to our socket helper. So remove the k_ prefix in order to avoid confusion. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
jukkar
force-pushed
the
bug-34964-socket-locking-regression
branch
from
09ff600 to
410c799
Compare
mniestroj
approved these changes
May 24, 2021
rlubos
approved these changes
May 24, 2021
jukkar
added
bug
Sorry for the delay with response, I was on vacation. Confirming that that this patch resolved issue with dumb_http_server/qemu_x86 + ab for me. Also retested with big_http_download (as the test for "opposite" transfer direction), and it works as expected. Thanks for investigating this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a regression when application is waiting data but does
not notice that because socket layer is not woken up.
This could happen because application was waiting condition
variable but the signal to wake the condvar came before the
wait started. Normally if there is constant flow of incoming
data to the socket, the signal would be given later. But if
the peer is waiting that Zephyr replies, there might be a
timeout at peer.
The solution is to add locking in socket receive callback so
that we only signal the condition variable after we have made
sure that the condition variable is actually waiting the data.
Fixes #34964
Signed-off-by: Jukka Rissanen jukka.rissanen@linux.intel.com