-
Notifications
You must be signed in to change notification settings - Fork 6
Perf array map impl #31
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
Conversation
af072a6
to
78e9c67
Compare
rex-macros/src/kprobe.rs
Outdated
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 don't think we should include the uprobe
support in this PR since it is not about uprobe
. Moreover, the change on the compiler is not included here so it cannot be used anyway.
rex/src/tracepoint/tp_impl.rs
Outdated
@@ -43,7 +49,28 @@ impl tracepoint { | |||
} | |||
|
|||
impl rex_prog for tracepoint { | |||
fn prog_run(&self, ctx: *mut ()) -> u32 { | |||
fn prog_run(&mut self, ctx: *mut ()) -> u32 { | |||
self.ctx_ptr = ctx as *const (); |
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 apparently this is one thing I missed in our previous discussions --- writing to the program object is not safe since it lives in the global data section, and we cannot synchronize this across multiple instances of the program running on different CPUs.
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 see. It looks like we need the enum then.
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.
Actually is there another way to load the context pointer into the program object, say at compile time?
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.
If you are referring to the address of the context, it is only available at runtime.
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.
Actually, the reason we cannot associate the ctx type with the program is because __rex_entry_tracepoint
is an extern "C"
function. Maybe we can completely bypass this layer of indirection by generating all the needed code in rex-macros
?
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 would the case where the user has multiple tracepoint programs to different hooks work then? Because then we will need to associate multiple context types with the tracepoint program type.
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.
We are going generate individual entry functions for each of the tp programs. The good thing is that each tp program can only associate with one hookpoint (forget about programs without hookpoints for now)
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.
Instead of messing with the macro and while avoiding duplicating code in an Enum, I think we can add a trait:
trait TracepointContext
and a newtype:
struct TpCtxWrapper<C: TracepointContext> {
ref: &'static C
}
and then associate this newtype with the tracepoint program.
Would this be better?
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.
But can this workaround the problem of extern "C" __rex_entry_tracepoint
? That function cannot be generic.
645680b
to
ebbd5d1
Compare
ebbd5d1
to
5d736db
Compare
d8ad0b0
to
f0e4c33
Compare
rex/src/utils.rs
Outdated
/// Programs that can stream data through a | ||
/// RexPerfEventArray will implement this trait | ||
pub trait PerfEventStreamer { | ||
type Context: ?Sized; |
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.
Nit: do we have any cases where the context is not Sized
?
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'm not sure, I'm just putting it there just in case. We can remove the bound for now.
f0e4c33
to
8bcc72b
Compare
Signed-off-by: MinhPhan8803 <[email protected]>
Signed-off-by: MinhPhan8803 <[email protected]>
Signed-off-by: MinhPhan8803 <[email protected]>
Signed-off-by: MinhPhan8803 <[email protected]>
Signed-off-by: MinhPhan8803 <[email protected]>
8bcc72b
to
80a2b48
Compare
Because #6 is getting quite bloated.
Add support for PERF_EVENT_ARRAY. No test yet, ideally Harpoon port will test this new map type.
Remaining question: is it safe to stick a raw pointer into a program type and just implement
Sync
for that program?