Skip to content

bpf:trace: pass L3 protocol to send_trace_notify #39794

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

Merged
merged 3 commits into from
May 30, 2025

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented May 29, 2025

Let's pass the L3 protocol to out send_trace_notify helper while emitting a trace notification.
This is useful to avoid re-computing it in case it is needed (see #39650 where we start differentiating protocols in ctx_classify).

@smagnani96 smagnani96 requested review from rgo3 and julianwiedmann May 29, 2025 18:06
@smagnani96 smagnani96 self-assigned this May 29, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 29, 2025
@smagnani96 smagnani96 added kind/enhancement This would improve or streamline existing functionality. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 29, 2025
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 changed the title bpf:trace: refactors helpers for codepath-specific calls bpf:trace: refactor helpers for protocol-specific usages May 29, 2025
@julianwiedmann
Copy link
Member

lgtm!

For some of the changes in bpf_lxc and bpf_overlay I could see us eventually converging to a send_trace_notify(..., __be16 ethertype, ...) variant. So that we don't have to create all these additional proto-specific calls, but can pass the additional information from just a single call-site. But that's for the future :)

Prior to this patch, using `send_trace_notify{4,6}` cause the metrics
to be updated using __MAGIC_LINE__ and __MAGIC_FILE__ from `trace.h`
rather than from the original file invoking the helper function.
That behavior is not aligned to `send_trace_notify` helper, which
makes use of the caller's line+file. Let's fix this while also slightly
refactors such helpers.

We keep the `update_trace_metrics()` macro so that we can decide to make
use of it instead of the whole trace notification in come codepaths
(future PR).

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commits adds to the encap-related helpers a new parameter `__be16 proto`
that corresponds to the L3 protocol carried. This will come useful in
subsequent commit, where we pass this parameter to the trace helpers.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/trace-refactors branch from 550afa1 to 4c59ca5 Compare May 30, 2025 10:29
@smagnani96
Copy link
Contributor Author

lgtm!

For some of the changes in bpf_lxc and bpf_overlay I could see us eventually converging to a send_trace_notify(..., __be16 ethertype, ...) variant. So that we don't have to create all these additional proto-specific calls, but can pass the additional information from just a single call-site. But that's for the future :)

I couldn't wait for the future knowing there was a way better solution.
Again, thanks so much for the review and the idea.
Here's the updated PR 😃

@smagnani96 smagnani96 changed the title bpf:trace: refactor helpers for protocol-specific usages bpf:trace: pass L3 protocol to send_trace_notify May 30, 2025
@smagnani96
Copy link
Contributor Author

/test

This commit adds to the `send_trace_notify` helper the `__be16 proto`
fields, which represents the L3 protocol carried. This will come useful
in subsequent PR for `ctx_classify` where, for instance, we could save
verifier complexity while parsing the packet to determine the traffic
pattern (e.g., overlay, wireguard, etc.) and compute the apposite
classifiers.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/trace-refactors branch from 4c59ca5 to 977ea24 Compare May 30, 2025 10:47
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 marked this pull request as ready for review May 30, 2025 12:53
@smagnani96 smagnani96 requested review from a team as code owners May 30, 2025 12:53
Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

Looks good for my codeowners!

Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

nice improvement!

@julianwiedmann julianwiedmann added the area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. label May 30, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 30, 2025
@julianwiedmann julianwiedmann added this pull request to the merge queue May 30, 2025
Merged via the queue into main with commit 4866264 May 30, 2025
347 of 350 checks passed
@julianwiedmann julianwiedmann deleted the pr/smagnani96/trace-refactors branch May 30, 2025 13:48
smagnani96 added a commit that referenced this pull request Jun 4, 2025
In #39794, we enabled passing the carried L3 protocol when calling
`send_trace_notify`. Let's use this argument for `ctx_classify`.
This allows to set the CLS_FLAG_IPV6 whenever we detect an IPV6 packet,
and not only when having a layer 3 IPv6 packet. In addition, this is
useful in subsequent commits where we extend classifiers: the new logic
will save verifier complexity and packet processing time given we have
already the protocol computed. For some edge case in which the variable
protocol is left to zero and not being set to the current protocol
(ex. from drop notification or when being called after a failing validate_ethertype),
we can compute the protocol from `ctx_classify` itself.

Here we also initialize the proto variable in bpf_lxc.c to avoid
compilation errors due to variable not being initialized. This was not
needed before as it was not being used, therefore removed during compilation.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
smagnani96 added a commit that referenced this pull request Jun 4, 2025
In #39794, we enabled passing the carried L3 protocol when calling
`send_trace_notify`. Let's use this argument for `ctx_classify`.
This allows to set the CLS_FLAG_IPV6 whenever we detect an IPV6 packet,
and not only when having a layer 3 IPv6 packet. In addition, this is
useful in subsequent commits where we extend classifiers: the new logic
will save verifier complexity and packet processing time given we have
already the protocol computed. For some edge case in which the variable
protocol is left to zero and not being set to the current protocol
(ex. from drop notification or when being called after a failing validate_ethertype),
we can compute the protocol from `ctx_classify` itself.

Here we also initialize the proto variable in bpf_lxc.c to avoid
compilation errors due to variable not being initialized. This was not
needed before as it was not being used, therefore removed during compilation.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2025
In #39794, we enabled passing the carried L3 protocol when calling
`send_trace_notify`. Let's use this argument for `ctx_classify`.
This allows to set the CLS_FLAG_IPV6 whenever we detect an IPV6 packet,
and not only when having a layer 3 IPv6 packet. In addition, this is
useful in subsequent commits where we extend classifiers: the new logic
will save verifier complexity and packet processing time given we have
already the protocol computed. For some edge case in which the variable
protocol is left to zero and not being set to the current protocol
(ex. from drop notification or when being called after a failing validate_ethertype),
we can compute the protocol from `ctx_classify` itself.

Here we also initialize the proto variable in bpf_lxc.c to avoid
compilation errors due to variable not being initialized. This was not
needed before as it was not being used, therefore removed during compilation.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2025
In #39794, we enabled passing the carried L3 protocol when calling
`send_trace_notify`. Let's use this argument for `ctx_classify`.
This allows to set the CLS_FLAG_IPV6 whenever we detect an IPV6 packet,
and not only when having a layer 3 IPv6 packet. In addition, this is
useful in subsequent commits where we extend classifiers: the new logic
will save verifier complexity and packet processing time given we have
already the protocol computed. For some edge case in which the variable
protocol is left to zero and not being set to the current protocol
(ex. from drop notification or when being called after a failing validate_ethertype),
we can compute the protocol from `ctx_classify` itself.

Here we also initialize the proto variable in bpf_lxc.c to avoid
compilation errors due to variable not being initialized. This was not
needed before as it was not being used, therefore removed during compilation.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2025
In #39794, we enabled passing the carried L3 protocol when calling
`send_trace_notify`. Let's use this argument for `ctx_classify`.
This allows to set the CLS_FLAG_IPV6 whenever we detect an IPV6 packet,
and not only when having a layer 3 IPv6 packet. In addition, this is
useful in subsequent commits where we extend classifiers: the new logic
will save verifier complexity and packet processing time given we have
already the protocol computed. For some edge case in which the variable
protocol is left to zero and not being set to the current protocol
(ex. from drop notification or when being called after a failing validate_ethertype),
we can compute the protocol from `ctx_classify` itself.

Here we also initialize the proto variable in bpf_lxc.c to avoid
compilation errors due to variable not being initialized. This was not
needed before as it was not being used, therefore removed during compilation.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2025
In #39794, we enabled passing the carried L3 protocol when calling
`send_trace_notify`. Let's use this argument for `ctx_classify`.
This allows to set the CLS_FLAG_IPV6 whenever we detect an IPV6 packet,
and not only when having a layer 3 IPv6 packet. In addition, this is
useful in subsequent commits where we extend classifiers: the new logic
will save verifier complexity and packet processing time given we have
already the protocol computed. For some edge case in which the variable
protocol is left to zero and not being set to the current protocol
(ex. from drop notification or when being called after a failing validate_ethertype),
we can compute the protocol from `ctx_classify` itself.

Here we also initialize the proto variable in bpf_lxc.c to avoid
compilation errors due to variable not being initialized. This was not
needed before as it was not being used, therefore removed during compilation.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants