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

libbpf-tools: add cpu filter for hardirqs/softirqs #5107

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ZhangYet
Copy link
Contributor

Since tools/hardirqs and tools/softirqs supports a --cpu option, it's reasonable to support a same option in libbpf version.

libbpf-tools/softirqs.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@yonghong-song yonghong-song left a comment

Choose a reason for hiding this comment

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

Please try to avoid unnecessary code format churns.

@ZhangYet ZhangYet force-pushed the dante-dev branch 10 times, most recently from cee19ff to 6bb7124 Compare September 28, 2024 12:17
libbpf-tools/hardirqs.c Outdated Show resolved Hide resolved
libbpf-tools/hardirqs.c Outdated Show resolved Hide resolved
u64 delta, *tsp;
u32 key = 0;

if (vec_nr >= NR_SOFTIRQS)
return 0;
vec_nr &= 0xf; // to avoid compiler opti make verifier error
Copy link
Collaborator

Choose a reason for hiding this comment

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

which compiler do you use and what kind of error you hit on which kernel?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please give details about the above verifier failure (kernel version, compiler version, error message, etc.)

Copy link
Contributor Author

@ZhangYet ZhangYet Sep 30, 2024

Choose a reason for hiding this comment

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

Hi @yonghong-song

I have replied your #5107 (comment) yesterday as below (but it shows pending I think that's why you didn't see it):

clang: 18.1.8
kernel: 6.10.10-arch1-1

Here is the error log:

if (vec_nr >= NR_SOFTIRQS)
        return 0;

https://gist.github.com/ZhangYet/5d293455680ef9953f7aed13a56463ac

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I missed the original comments.
From the verifier log, the following is the reason:

18: (bf) r1 = r6                      ; frame1: R1_w=scalar(id=1) R6=scalar(id=1)
19: (67) r1 <<= 32                    ; frame1: R1_w=scalar(smax=0x7fffffff00000000,umax=0xffffffff00000000,smin32=0,smax32=umax32=0,var_off=(0x0; 0xffffffff00000000))
20: (77) r1 >>= 32                    ; frame1: R1_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
21: (25) if r1 > 0x9 goto pc+72       ; frame1: R1_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=9,var_off=(0x0; 0xf))
22: (bf) r2 = r10                     ; frame1: R2_w=fp0 R10=fp0
...
39: (67) r6 <<= 32                    ; frame1: R6_w=scalar(smax=0x7fffffff00000000,umax=0xffffffff00000000,smin32=0,smax32=umax32=0,var_off=(0x0; 0xffffffff00000000))
40: (77) r6 >>= 32                    ; frame1: R6_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
...

In the above, at insn 22, we know r1 <= 0x9, and not that insn 18, we have r1 = r6, so lower 32bit of r6 should be <= 0x9 as well.
After insn 40, we know r6 top 32bits are 0, so in ideal case verifier should conclude r6 <= 0x9, but unfortunately verifier is not sophisticated enough to derive the precise bound.

I suspect -mcpu=v3 might work as we could remove some left/right shifts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. -mcpu=v3 works well.

I added

BPFCFLAGS_softirqs := $(BPFCFLAGS) -mcpu=v3
$(OUTPUT)/softirqs.bpf.o: BPFCFLAGS = $(BPFCFLAGS_softirqs)

I wonder if there is a better way to add -mcpu=v3 for softirqs. If so, I will modify and rebase the commit.

libbpf-tools/softirqs.c Outdated Show resolved Hide resolved
@ZhangYet ZhangYet force-pushed the dante-dev branch 3 times, most recently from e44323a to 9812c9b Compare September 30, 2024 09:28
libbpf-tools/softirqs.bpf.c Show resolved Hide resolved
u64 delta, *tsp;
u32 key = 0;

if (vec_nr >= NR_SOFTIRQS)
return 0;
vec_nr &= 0xf; // to avoid compiler opti make verifier error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please give details about the above verifier failure (kernel version, compiler version, error message, etc.)

libbpf-tools/softirqs.c Outdated Show resolved Hide resolved
libbpf-tools/hardirqs.bpf.c Outdated Show resolved Hide resolved
@ZhangYet ZhangYet force-pushed the dante-dev branch 2 times, most recently from cfe636f to 4159296 Compare October 1, 2024 03:04
Copy link
Collaborator

@yonghong-song yonghong-song left a comment

Choose a reason for hiding this comment

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

There are a lot of format issues as well. Please check

  • leading 'tab' instead of 'space'.
  • some places having trailing spaces.
    Please fix.

libbpf-tools/hardirqs.bpf.c Outdated Show resolved Hide resolved
libbpf-tools/softirqs.c Outdated Show resolved Hide resolved
tools/softirqs.py supports `-c/--cpu` otption to filter cpu,
It's reasonable to support a same otption for libbpf-tools/softirqs.

Add `-mcpu=v3` in Makefile for softirqs or the shift op will lead
to verifier errors.

Reported-by: Tang Yizhou <[email protected]>
Signed-off-by: Dantezy <[email protected]>
@ZhangYet
Copy link
Contributor Author

ZhangYet commented Oct 2, 2024

There are a lot of format issues as well. Please check

  • leading 'tab' instead of 'space'.
  • some places having trailing spaces.
    Please fix.

Sorry for the format issues. I kept switching development env between office and home, sometimes the fix was forget to push(like the space between { and cpu). I have replace all 8 space with tabs now.

Maybe we should provide a tool like checkpatch.el to do the format work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants