Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usrsock: Do not return error when conn not found for an event #8609

Merged
merged 2 commits into from Feb 21, 2023

Conversation

wengzhe
Copy link
Contributor

@wengzhe wengzhe commented Feb 21, 2023

Summary

When rpmsg is in heavy load(e.g. rpmsgfs), it may result in out-of-ordered messages because it can trigger another callback in rpmsg_send. Then client side of usrsock may receive an event after socket is closed (e.g. SENDTO_READY event in UDP connect). Normally just drop these events might be acceptable, but we're returning error now, and triggers RPMSG_ASSERT outside.

Patches included:

  • usrsock: Do not return error when conn not found for an event
    • If we receive an event to a conn after it's freed, log error and ignore it.
    • Also change log level to err when returning other negated errno, because if we return negated errno, we'll directly trigger RPMSG_ASSERT outside, so we need at least an error message to indicate the reason.
  • usrsock: Add DebugAssert for poll setup result
    • We never check usrsock_rpmsg_poll_setup's result and assume it will success, so an assert should be better than retval.

Impact

  • logic change: don't return error when usrsock conn not found for an event
  • robustness: logs / debug asserts

Testing

CI & Vela

We never check its result and assume it will success, so an assert should be better than retval.

Signed-off-by: Zhe Weng <wengzhe@xiaomi.com>
There might be some cases that we receive an event after socket is closed because of out-of-ordered rpmsg, since it may not cause problem, we may omit the error.
Also change log level to err when returning negated errno, because if we return negated errno, we'll directly trigger RPMSG_ASSERT outside, so we need at least an error message to indicate the reason.

Signed-off-by: Zhe Weng <wengzhe@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor

Let's ignore the ci broken which is fixed by #8611 and merge this simple change.

@xiaoxiang781216 xiaoxiang781216 merged commit 31b6584 into apache:master Feb 21, 2023
@jerpelea jerpelea added this to To-Add in Release Notes - 12.1.0 Apr 3, 2023
@jerpelea jerpelea moved this from To-Add to Added in Release Notes - 12.1.0 Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants