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.

Sign up for GitHub

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

Jump to bottom

net: Add timeout to various send functions #86127

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

net: Add timeout to various send functions #86127

wants to merge 5 commits into from
+92 −31

Conversation

clamattia
Copy link
Contributor

@clamattia clamattia commented Feb 21, 2025 •
edited
Loading

This is just to discuss the idea. I can bring into better format, if we agree on approach.

Basic idea is adding timeout to various send function. To keep stable API, functions are renamed with _try-prefix. Original functions just call the _try-variant with timeout K_FOREVER.

Fixes: #85981

Timeout can be used to early drop some request for arp and icmp to avoid being slowed when in a flood situation.

Looking forward to your thoughts.

rlubos
rlubos reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could go this way if we retain stable APIs compability.

pdgendt
pdgendt reviewed Feb 21, 2025
View reviewed changes
subsys/net/ip/net_core.c Outdated
Comment on lines 366 to 369
int net_send_data(struct net_pkt *pkt)
{
return net_try_send_data(pkt, K_FOREVER);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert this to an inline function instead?

@clamattia clamattia changed the title net: RFC: Add timeout to various send functions net: Add timeout to various send functions Feb 21, 2025
@clamattia
Copy link
Contributor Author

  • Created separate commits for introducing timeout and using it

  • made original functions static inline as suggested by @pdgendt

  • For now I added timeout for net_context, arp, icmp, lldp

rlubos
rlubos reviewed Feb 21, 2025
View reviewed changes
subsys/net/ip/net_context.c
@@ -2609,7 +2609,7 @@ static int context_sendto(struct net_context *context,
net_pkt_set_ll_proto_type(pkt,
ntohs(ll_dst_addr->sll_protocol));

net_if_queue_tx(net_pkt_iface(pkt), pkt);
net_if_try_queue_tx(net_pkt_iface(pkt), pkt, timeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context_sendto() has actually more places where data is passed to the lower layer (net_send_data() calls above and below).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@clamattia
Copy link
Contributor Author

Added timeouts to more send functions in context_sendto.

@clamattia
Copy link
Contributor Author

Fix compliance issues.

@clamattia
Copy link
Contributor Author

  • Fix silly mistake
  • Document timeout parameter

@clamattia
Copy link
Contributor Author

Rebased

@clamattia
Copy link
Contributor Author

Reorder declarations in header files, so they are known for static inline functions.

pdgendt
pdgendt previously approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good addition to me.

@clamattia
Copy link
Contributor Author

clamattia commented Feb 26, 2025 •
edited
Loading

  • Fix some CI by using correct k_timeout_t type
  • Remove no longer needed timeout to K_NO_WAIT in ISR context, because arp and icmp set to K_NO_WAIT in calling context. @rlubos do you agree?

@rlubos
Copy link
Contributor

rlubos commented Feb 26, 2025

Remove no longer needed timeout to K_NO_WAIT in ISR context, because arp and icmp set to K_NO_WAIT in calling context. @rlubos do you agree?

I'd personally keep it just in case. Just realized, IPv6 ND has probably a similar scenario not handled here (NA reply for NS, in ipv6_nbr.c), maybe there are more cases.

@clamattia
Copy link
Contributor Author

Remove no longer needed timeout to K_NO_WAIT in ISR context, because arp and icmp set to K_NO_WAIT in calling context. @rlubos do you agree?

I'd personally keep it just in case. Just realized, IPv6 ND has probably a similar scenario not handled here (NA reply for NS, in ipv6_nbr.c), maybe there are more cases.

Will add it to the variatn without try.
Makes sense. I just implemented for the ones I was familiar with.

@clamattia
Copy link
Contributor Author

  • Tried to fix more ci-/compliance-problems
  • Added @rlubos ISR fix back

@clamattia
Copy link
Contributor Author

More compliance/ci fixes

@clamattia
Copy link
Contributor Author

Rebased

@clamattia
net: add try variant with timeout for various send functions
24b2a67 
Allows to send with different timeouts to not block caller in some
situations. Stable API is kept and just calls `try`-variant with a timeout
of `K_FOREVER`.

Signed-off-by: Cla Mattia Galliard <[email protected]>
@clamattia
net: net_context: Make use of timeout for sending
051ec53 
Use the newly added timeout in various send functions from within
net_context_sendto.

Signed-off-by: Cla Mattia Galliard <[email protected]>
@clamattia
net: arp: Send replies with timeout K_NO_WAIT
3fe3f8b 
This ensures system stays operational in arp-flood situation.

Signed-off-by: Cla Mattia Galliard <[email protected]>
@clamattia
net: lldp: Send replies with timeout K_NO_WAIT
05229a8 
This ensures system stays operation in lldp-flood situation.

Signed-off-by: Cla Mattia Galliard <[email protected]>
@clamattia
net: icmp: Send replies with timeout K_NO_WAIT
5309999 
This ensure system stays operational in icmp flood situation.

Signed-off-by: Cla Mattia Galliard <[email protected]>
@clamattia
Copy link
Contributor Author

Fixed silly mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlubos rlubos rlubos left review comments

@pdgendt pdgendt pdgendt left review comments

@jukkar jukkar Awaiting requested review from jukkar

@ssharks ssharks Awaiting requested review from ssharks

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

At least 2 approving reviews are required to merge this pull request.

Assignees

@jukkar jukkar

@rlubos rlubos

Labels
area: Networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

net: arp/icmp: throttle amount of request handled
5 participants
@clamattia @rlubos @pdgendt @jukkar @zephyrbot