-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix BCC CI: shallow-clone versioning, clang/tools_smoke test regressions #5428
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
base: master
Are you sure you want to change the base?
Conversation
a0c8bfc to
833a594
Compare
833a594 to
d2b204e
Compare
|
Updated |
tests/python/test_clang.py
Outdated
| if (!rq->rq_disk || rq->rq_disk->major != 5 || | ||
| rq->rq_disk->first_minor != 6) | ||
| struct gendisk *disk = rq->q->disk; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a NULL check needed for rq->q?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is used as a standalone test, so it isn’t attached to any events.
In normal blk-mq paths blk_mq_start_request, rq->q should always be initialized, so a NULL here would indicate a kernel issue.
Still, if you think adding a NULL check improves clarity, I’m happy to include it.
tests/python/test_clang.py
Outdated
| if (!rq->rq_disk || rq->rq_disk->major != 5 || | ||
| rq->rq_disk->first_minor != 6) | ||
| struct gendisk *disk = rq->q->disk; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow declaration-first style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. But to follow the rq->q NULL check, I’ll drop the separate declaration.
| text = b""" | ||
| #include <linux/blk_types.h> | ||
| #include <linux/blkdev.h> | ||
| #include <linux/blk-mq.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this patch require updating the @skipUnless kernel version requirement at line 1297?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I’ll update the @skipUnless accordingly.
| #include <linux/mm_types.h> | ||
| int test(struct pt_regs *ctx, struct mm_struct *mm) { | ||
| return mm->rss_stat.count[MM_ANONPAGES].counter; | ||
| return mm->rss_stat[MM_ANONPAGES].count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the rss_stat test need @skipUnless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll adjust as suggested.
| struct leaf { size_t size; }; | ||
| BPF_HASH(simple_map, u32, struct leaf); | ||
| int kprobe____kmalloc(struct pt_regs *ctx, size_t size) { | ||
| int kprobe____kmalloc_noprof(struct pt_regs *ctx, size_t size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the kmalloc_noprof test need @skipUnless as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I’ll update the @skipUnless accordingly.
cmake/version.cmake
Outdated
| set(GIT_TAG_EXACT "") | ||
| if(GIT_TAG_LAST MATCHES "-NOTFOUND") | ||
| set(REVISION "0.0.0+${GIT_SHA1_SHORT}") | ||
| message(STATUS "Using PEP 440 compliant version for shallow clone") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making the message more informative? e.g.,
message(STATUS "No valid tag reachable for shallow clone, using fallback 0.0.0+${GIT_SHA1_SHORT}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Since a tarball isn’t a shallow clone, I’ll adjust it to:
"No valid Git tag found, using fallback 0.0.0+${GIT_SHA1_SHORT}"
Thanks!
It's good. |
38550b3 to
d545980
Compare
bcc's GitHub workflow has been failing for a while when building from a shallow clone (git clone --depth 1). In this case, git_describe() from CMake returns an invalid version string (128-NOTFOUND-deadbeef). Older setuptools versions used in Fedora 38 tolerated this format, but with Ubuntu 24.04's newer setuptools enforces PEP 440 compliance and treats it as an error, breaking the CI builds. This updates cmake/version.cmake to detect this case and fall back to a compliant 0.0.0+<hash> version when tags are missing. Commit hash is kept for traceability and behavior with full clone hasn't changed. The '-NOTFOUND' placeholder is used to cover other edge cases defined in cmake/GitRevisionDescription.cmake. Signed-off-by: Hoyeon Lee <[email protected]>
Linux kernel commit f3fa33acca9f ("block: remove the ->rq_disk field in
struct request") dropped rq_disk in favor of rq->q->disk. The clang
tests still include <linux/blkdev.h> and read rq->rq_disk, which fails
on recent kernels.
16: include/linux/blkdev.h:34:8: note: forward declaration of 'struct request'
16: struct request;
16: ^
16: /virtual/main.c:15:12: error: incomplete definition of type 'struct request'
16: if (!rq->rq_disk || rq->rq_disk->major != 5 ||
16: ~~^
Since blk-mq became default in linux 5.x, this commit includes
<linux/blk-mq.h> and fetch the disk via rq->q->disk so the tests stay
valid on current kernels.
Signed-off-by: Hoyeon Lee <[email protected]>
Linux Kernel commit f1a7941243c1 ("mm: convert mm's rss stats into
percpu_counter") replaced the old rss_stat.count[] layout. The clang
test still dereferences mm->rss_stat.count[MM_ANONPAGES].counter, which
is old approach.
16: E../virtual/main.c:4:24: error: member reference base type 'struct percpu_counter[4]' is not a structure or union
16: return mm->rss_stat.count[MM_ANONPAGES].counter;
16: ~~~~~~~~~~~~^~~~~~
16: 1 error generated.
This commit change access with mm->rss_stat[MM_ANONPAGES].count instead
so the test stay valid on modern kernels.
Signed-off-by: Hoyeon Lee <[email protected]>
As part of the Memory Allocation Profiling patchset, Linux commit
7bd230a26648 ("mm/slab: enable slab allocation tagging for kmalloc and
friends") converted __kmalloc into a wrapper and moved the
actual implementation to __kmalloc_noprof, breaking kmalloc kprobes.
This commit update the clang/tools_smoke python test to attach to
__kmalloc_noprof instead, restoring the kmalloc kprobe test behavior
stay valid on modern kernels.
Signed-off-by: Hoyeon Lee <[email protected]>
btrfsdist/btrfsslower/f2fsslower fails on systems without the specific modules. Check for a loaded btrfs or f2fs module before invoking the scripts so the smoke test only runs when specific module is loaded. Signed-off-by: Hoyeon Lee <[email protected]>
The smoke tool test runs tool directly, and especially softirqslower.py
lacked the exec bit and failed with "Permission denied". Set the mode
to 0755 so the script can be executed by the tests.
29: ....
timeout: failed to run command ‘/bcc/tools/softirqslower.py’: Permission denied
Signed-off-by: Hoyeon Lee <[email protected]>
d545980 to
12c2beb
Compare
bcc CI has been red from shallow-clone builds emitting PEP 440–invalid
"128-NOTFOUND-…" versions, and from py_test_clang/py_test_tools_smoke
breakage on newer kernels. This series supersedes the prior PR #5414.
By first commit, cmake/version.cmake now falls back to a compliant
'0.0.0+<hash>' when tags are missing, satisfying newer python setuptools
in shallow-clone workflows.
The clang tests are fixed to use blk-mq (rq->q->disk in place of rq_disk)
and the percpu-counter layout for mm->rss_stat[MM_ANONPAGES].count,
and kmalloc kprobe are replaced with kmalloc_noprof for recent kernels.
[1] [2] [3]
Additionally, smoke tests now only runs btrfs/f2fs tools when those
modules are loaded, and softirqslower.py is fixed with missing exec bit to
avoid error.
Together these changes clear the py_test_clang and py_test_tools_smoke
failures and restore CI.
1: commit torvalds/linux@f3fa33acca9f ("block: remove the ->rq_disk field in struct request")
2: commit torvalds/linux@f1a7941243c1 ("mm: convert mm's rss stats into percpu_counter")
3: commit torvalds/linux@7bd230a26648 ("mm/slab: enable slab allocation tagging for kmalloc and friends")