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

net/arp: modify some flow of arp return failure #13714

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zhhyu7
Copy link
Contributor

@zhhyu7 zhhyu7 commented Sep 29, 2024

Summary

If arp search fails once, subsequent searches for the ip will directly return failure, and sends an asynchronous arp request to try to update arp table in the future. In this way, the psock_sendmsg interface will not block for a long time each time because arp cannot be obtained.

This scenario is triggered when a udp socket frequently attempts to access an ip address that does not exist on the LAN.

Impact

Testing

sim:local

If arp search fails once, subsequent searches for the ip will directly
return failure, and sends an asynchronous arp request to try to update
arp table in the future. In this way, the psock_sendmsg interface will
not block for a long time each time because arp cannot be obtained.

This scenario is triggered when a udp socket frequently attempts to
access an ip address that does not exist on the LAN.

Signed-off-by: zhanghongyu <zhanghongyu@xiaomi.com>
@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Sep 29, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

Let's break down this Pull Request (PR) against the NuttX requirements:

Summary:

  • Why? The PR aims to prevent prolonged blocking in the psock_sendmsg function when a UDP socket repeatedly tries to send data to an unreachable IP address. This is a performance and responsiveness improvement.
  • What? Modifies the ARP (Address Resolution Protocol) handling to cache ARP failures. If an ARP lookup fails, subsequent attempts will fail immediately, but an asynchronous ARP request will be sent in the background to potentially update the cache.
  • How? Introduces ARP failure caching logic. Instead of blocking indefinitely on ARP resolution, the first failure for an IP is cached, and subsequent attempts will return the cached failure. A background ARP request attempts to refresh the entry.
  • Issue References: The PR summary lacks links to relevant NuttX issues. This makes it harder to assess if the problem was previously reported/discussed.

Impact:

  • New Feature? Existing Change? This is an optimization/bug fix to existing ARP handling, not a new feature.
  • User Impact: Likely minimal for most users. UDP communication to non-existent IPs might see a slight initial delay, followed by faster failure handling. This could be beneficial for applications relying on fast UDP communication.
  • Build Impact: Unclear. The PR description doesn't mention any changes to the build process. Assuming no changes, the impact here is likely minimal.
  • Hardware Impact: Unlikely. ARP is a network layer protocol, so this change shouldn't directly impact specific hardware or drivers.
  • Documentation Impact: The PR should ideally include documentation updates to reflect the new ARP caching behavior. This could be in the form of code comments or updates to relevant documentation files.
  • Security Impact: Potentially minor. ARP caching can be susceptible to ARP poisoning attacks. The PR description doesn't specify if any mitigations against such attacks are included.
  • Compatibility Impact: Potentially minor. Applications relying on very specific ARP behavior (e.g., expecting prolonged blocking on ARP resolution) might be affected. However, this is unlikely to be a common use case.

Testing:

  • Insufficient Detail: The testing section is too vague. It only mentions "sim:local" which doesn't provide enough information to understand the testing environment.

Does the PR meet NuttX Requirements?

The PR partially meets the requirements. Here's a breakdown:

  • Strengths:
    • Clearly identifies a performance bottleneck.
    • Provides a reasonable solution to improve responsiveness.
  • Weaknesses:
    • Lacks references to related NuttX issues.
    • Testing description is insufficient.
    • Impact on documentation, security, and compatibility is not adequately addressed.

Recommendations for Improvement:

  1. Link to Issues: Provide links to any relevant NuttX issues that prompted this change or discuss the problem being solved.
  2. Detailed Testing: Describe the testing environment thoroughly:
    • Simulator used (e.g., QEMU)
    • NuttX configuration
    • Steps to reproduce the issue and verify the fix
    • Include relevant log snippets (with clear "before" and "after" sections)
  3. Address Impacts: Expand on the potential impacts:
    • Documentation: State whether documentation updates are included or planned.
    • Security: Discuss any potential security implications and mitigations taken (if any).
    • Compatibility: Elaborate on any potential compatibility concerns and how they are addressed.

By providing these clarifications, the PR will be more robust and better adhere to the NuttX contribution guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants