-
Notifications
You must be signed in to change notification settings - Fork 685
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
perf: clean up tests #979
perf: clean up tests #979
Conversation
cc @eiffel-fl |
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.
Thank you for it! It makes the code clearer and each test is more specific which I think is a good point.
I just have one question regarding the reading function.
// padding is an implementation detail of the perf buffer and 1-7 bytes long. The | ||
// contents are undefined. |
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 am not sure to understand.
Here you say the content is undefined but later in the code you test the padding to be 0.
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.
Ah yes, that was based on a discussion with @ti-mo. Seems like padding is zero after all?
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 think this is related to this:
Line 310 in 5ea5368
// Records can contain between 0 and 7 bytes of trailing garbage from the ring |
If I remember, someone shared the kernel commit in the previous PR, I will try to find it.
Indeed, I would like to know if this is always zero, in this case we can keep this testing.
Otherwise, better to remove it and we are just lucky here 😅.
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.
Padding is not zeroed by the kernel and will contain garbage after it's wrapped the ring. I've revived this thread recently: https://lore.kernel.org/lkml/20200519132616.794171-1-timo@incline.eu as it was never merged.
See #94 (comment) for full context. I'd keep the test to make sure we're not making mistakes elsewhere.
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've revived this thread recently: https://lore.kernel.org/lkml/20200519132616.794171-1-timo@incline.eu as it was never merged.
You are doing right as this would be a great addition to merge!
I'd keep the test to make sure we're not making mistakes elsewhere.
We then need to modify the test as it is currently testing that padding is 0?
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.
Padding is not zeroed by the kernel and will contain garbage after it's wrapped the ring.
That's what's confusing me. We wrap the buffer in tests, and we check that the padding is zero. Yet the test doesn't fail. So either we're really lucky and don't get garbage by chance (how? maybe because our sample sizes don't have padding?) or it actually gives zero.
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.
Okay, the reason we don't see it in tests is that we never overwrite data in the regular perf buffer case. For the overwritable buffer I'm not so sure, maybe it's because the samples fit exactly?
The regular and overwritable perf buffer tests currently don't share much test scaffolding because we want to know which sample we've read for the overwritable case. Reduce code duplication by adopting the regular and overwritable tests to use a common sample format which includes both size and sample ID. Also adopt some of the helpers to fit their most common use case. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
The regular and overwritable perf buffer tests currently don't share much test scaffolding because we want to know which sample we've read for the overwritable case.
Reduce code duplication by adopting the regular and overwritable tests to use a common sample format which includes both size and sample ID. Also adopt some of the helpers to fit their most common use case.