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

datapath: Introduce helpers for __ctx_is checks #23008

Closed
brb opened this issue Jan 10, 2023 · 6 comments · Fixed by #23820
Closed

datapath: Introduce helpers for __ctx_is checks #23008

brb opened this issue Jan 10, 2023 · 6 comments · Fixed by #23820
Assignees
Labels
good-first-issue Good starting point for new developers, which requires minimal understanding of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.

Comments

@brb
Copy link
Member

brb commented Jan 10, 2023

In the datapath we have a few __ctx_is checks:

> git grep '__ctx_is =='
bpf/include/bpf/ctx/common.h:    return __ctx_is == __ctx_xdp;
bpf/include/bpf/helpers.h:#if __ctx_is == __ctx_skb
bpf/lib/edt.h:#if defined(ENABLE_BANDWIDTH_MANAGER) && __ctx_is == __ctx_skb
bpf/lib/identity.h:#if __ctx_is == __ctx_skb
bpf/lib/identity.h:#endif /* __ctx_is == __ctx_skb */
bpf/lib/nodeport.h:#if __ctx_is == __ctx_skb
bpf/lib/overloadable.h:#if __ctx_is == __ctx_skb
bpf/lib/pcap.h:# define capture_enabled (__ctx_is == __ctx_xdp)
bpf/lib/proxy.h:#if !(__ctx_is == __ctx_skb)
bpf/lib/qm.h:#if defined(RESET_QUEUES) && __ctx_is == __ctx_skb

For the sake of readability, we should introduce ctx_is_{skb,xdp}() helpers, and replace the ifdef's with the if statements.

@brb brb added good-first-issue Good starting point for new developers, which requires minimal understanding of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jan 10, 2023
@SamrathPalSingh
Copy link

/assign

@brb
Copy link
Member Author

brb commented Jan 30, 2023

@SamrathPalSingh Are you still interested in working on this issue?

@spacewander
Copy link
Contributor

Hi @brb, I am interested in it.

For the sake of readability, we should introduce ctx_is_{skb,xdp}() helpers, and replace the ifdef's with the if statements.

Do you mean ctx_is_skb macro? There is a guard macro in

#if __ctx_is == __ctx_skb
# include "helpers_skb.h"
#else
# include "helpers_xdp.h"
#endif

A runtime if with function call may not support including different files.

@brb
Copy link
Member Author

brb commented Feb 13, 2023

Hi @spacewander . I meant the following:

bool ctx_is_skb() {
    return __ctx_is == __ctx_skb
}

And then replace #if __ctx_is == __ctx_skb with if ctx_is_skb() { .... Same goes for the XDP.

@spacewander
Copy link
Contributor

Yes. But this doesn't allow selecting #include <file> dynamically, as shown in #23008 (comment).
Do we need to keep that pattern unchanged?

@brb
Copy link
Member Author

brb commented Feb 14, 2023

Do we need to keep that pattern unchanged?

Ah, I see. Yep, let's keep it unchanged.

spacewander added a commit to spacewander/cilium that referenced this issue Jun 16, 2023
Only macros in function body are updated so it doesn't
affect function definitions.

Fixes: cilium#23008

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
borkmann pushed a commit that referenced this issue Jun 20, 2023
Only macros in function body are updated so it doesn't
affect function definitions.

Fixes: #23008

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
romanspb80 pushed a commit to romanspb80/cilium that referenced this issue Jun 22, 2023
Only macros in function body are updated so it doesn't
affect function definitions.

Fixes: cilium#23008

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good starting point for new developers, which requires minimal understanding of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants