| Message ID | 20240318163552.3808695-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/trace: Treewide API cleanup | expand |
On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > trace() and trace_time(), in function form for struct arguments, and macro > form for simple uint32_t list arguments. > > This will be used to clean up the mess of macros which exists throughout the > codebase, as well as eventually dropping __trace_var(). > > There is intentionally no macro to split a 64-bit parameter in the new API, > for MISRA reasons. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks for doing this, it all looks good to me. I'll note that last time this was posted, there were some questions. One was regarding underscores in the macro parameters. I don't care about this enough to stop it going in. The other question was about the use of ##__VA_ARGS__. I don't actually understand what that comment was trying to say, as a quick Google search it appears that 1) this will do what I think it should do, and 2) it works both in gcc and clang. We should give Jan a chance to explain his second point more clearly before checking it in; but: Reviewed-by: George Dunlap <george.dunlap@cloud.com>
On 20.03.2024 12:58, George Dunlap wrote: > On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> trace() and trace_time(), in function form for struct arguments, and macro >> form for simple uint32_t list arguments. >> >> This will be used to clean up the mess of macros which exists throughout the >> codebase, as well as eventually dropping __trace_var(). >> >> There is intentionally no macro to split a 64-bit parameter in the new API, >> for MISRA reasons. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Thanks for doing this, it all looks good to me. > > I'll note that last time this was posted, there were some questions. > One was regarding underscores in the macro parameters. I don't care > about this enough to stop it going in. We ought to finally settle on our take on this, preferably recording the outcome in ./CODING_STYLE. > The other question was about the use of ##__VA_ARGS__. I don't > actually understand what that comment was trying to say, as a quick > Google search it appears that 1) this will do what I think it should > do, and 2) it works both in gcc and clang. > > We should give Jan a chance to explain his second point more clearly > before checking it in; Well. Gcc 13.2 documentation explicitly mentions this form; interestingly with a quick search I couldn't find the original extension's documentation anymore. It is possible that I didn't spot this 2.5 years ago. It is also possible the their doc has changed in the meantime. In any event, my view back at the time was that I'd preferably see us use the (long-standing) gcc extension (args...) or use __VA_ARGS__ in a standard compliant way. Yet with the original form no longer properly documented, maybe they have a plan to remove that at some point. So while my preference stays for the time being, I also don't mind if this code is left as is. Jan > but: > > Reviewed-by: George Dunlap <george.dunlap@cloud.com>
diff --git a/xen/common/trace.c b/xen/common/trace.c index 4e7b080e6154..c94ce1f7dc90 100644 --- a/xen/common/trace.c +++ b/xen/common/trace.c @@ -808,6 +808,11 @@ void __trace_var(u32 event, bool cycles, unsigned int extra, tasklet_schedule(&trace_notify_dom0_tasklet); } +void trace(uint32_t event, unsigned int extra, const void *extra_data) +{ + __trace_var(event, event & TRC_HD_CYCLE_FLAG, extra, extra_data); +} + void __trace_hypercall(uint32_t event, unsigned long op, const xen_ulong_t *args) { diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h index 055883287e8c..f184844e1b55 100644 --- a/xen/include/xen/trace.h +++ b/xen/include/xen/trace.h @@ -37,6 +37,9 @@ int tb_control(struct xen_sysctl_tbuf_op *tbc); int trace_will_trace_event(u32 event); +/* Create a trace record, with pre-constructed additional parameters. */ +void trace(uint32_t event, unsigned int extra, const void *extra_data); + void __trace_var(uint32_t event, bool cycles, unsigned int extra, const void *); static inline void trace_var(uint32_t event, bool cycles, unsigned int extra, @@ -66,6 +69,9 @@ static inline int trace_will_trace_event(uint32_t event) return 0; } +static inline void trace( + uint32_t event, unsigned int extra, const void *extra_data) {} + static inline void trace_var(uint32_t event, bool cycles, unsigned int extra, const void *extra_data) {} static inline void __trace_var(uint32_t event, bool cycles, unsigned int extra, @@ -74,6 +80,30 @@ static inline void __trace_hypercall(uint32_t event, unsigned long op, const xen_ulong_t *args) {} #endif /* CONFIG_TRACEBUFFER */ +/* Create a trace record with time included. */ +static inline void trace_time( + uint32_t event, unsigned int extra, const void *extra_data) +{ + trace(event | TRC_HD_CYCLE_FLAG, extra, extra_data); +} + +/* + * Create a trace record, packaging up to 7 additional parameters into a + * uint32_t array. + */ +#define TRACE(_e, ...) \ + do { \ + if ( unlikely(tb_init_done) ) \ + { \ + uint32_t _d[] = { __VA_ARGS__ }; \ + BUILD_BUG_ON(ARRAY_SIZE(_d) > TRACE_EXTRA_MAX); \ + trace(_e, sizeof(_d), sizeof(_d) ? _d : NULL); \ + } \ + } while ( 0 ) + +/* Create a trace record with time included. */ +#define TRACE_TIME(_e, ...) TRACE((_e) | TRC_HD_CYCLE_FLAG, ##__VA_ARGS__) + /* Convenience macros for calling the trace function. */ #define TRACE_0D(_e) \ do { \
trace() and trace_time(), in function form for struct arguments, and macro form for simple uint32_t list arguments. This will be used to clean up the mess of macros which exists throughout the codebase, as well as eventually dropping __trace_var(). There is intentionally no macro to split a 64-bit parameter in the new API, for MISRA reasons. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> CC: consulting@bugseng.com <consulting@bugseng.com> v3: * Extend with trace() and trace_time() * Delete TRACE_PARAM64() for MISRA reasons. --- xen/common/trace.c | 5 +++++ xen/include/xen/trace.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)