Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[chenbaodong <chenbaodong@xxxxxxxxxx>]

On 5/31/19 19:10, Jan Beulich wrote:
> > > > On 30.05.19 at 12:17, <chenbaodong@xxxxxxxxxx> wrote:
> > Default: enabled.
> > Can be disabled for smaller code footprint.
> But you're aware that we're, for now at least, trying to limit the
> number of independently selectable config options? Ones depending
> on EXPERT are sort of an exception in certain cases.
Limit the number of independently selectable config sounds good to me.

Does the following looks good?

+config HAS_TRACEBUFFER
+       bool "Enable/Disable tracebuffer"  if EXPERT = "y"
+       ---help---
+         Enable or disable tracebuffer function.
+         Xen internal running status(trace event) will be saved to 
trace memory
+         when enabled.
+

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -368,4 +368,10 @@ config DOM0_MEM
> >   
> >   	  Leave empty if you are not sure what to specify.
> >   
> > +config HAS_TRACEBUFFER
> > +       bool "Enable/Disable tracebuffer"
> > +       default y
> > +       ---help---
> > +         Enable or disable tracebuffer function.
> HAS_* options should not come with a prompt, and should
> only be "select"-ed by (normally) per-architecture Kconfig
> files. If we are to permit having this option, then I also think
> the help text needs expanding.
Tanks for your clarification.
> > --- a/xen/include/xen/trace.h
> > +++ b/xen/include/xen/trace.h
> > @@ -21,12 +21,14 @@
> >   #ifndef __XEN_TRACE_H__
> >   #define __XEN_TRACE_H__
> >   
> > -extern int tb_init_done;
> >   
> >   #include <public/sysctl.h>
> >   #include <public/trace.h>
> >   #include <asm/trace.h>
> >   
> > +#ifdef CONFIG_HAS_TRACEBUFFER
> > +
> > +extern int tb_init_done;
> >   /* Used to initialise trace buffer functionality */
> >   void init_trace_bufs(void);
> I wonder if there hadn't been a reason for the declaration to live
> up where it was. Also please separate independent blocks of code
> by a blank line.
Roger that.
> > @@ -47,6 +49,20 @@ static inline void trace_var(u32 event, int cycles, int
> > extra,
> >   void __trace_hypercall(uint32_t event, unsigned long op,
> >                          const xen_ulong_t *args);
> >   
> > +#else
> > +#define tb_init_done (0)
> Perhaps better "false", and no real need for the parentheses afaict.
>
> > +static inline void init_trace_bufs(void) {}
> > +static inline int tb_control(struct xen_sysctl_tbuf_op *tbc) { return -ENOSYS; 
> > }
> > +
> > +static inline int trace_will_trace_event(u32 event) { return 0; }
> > +static inline void trace_var(u32 event, int cycles, int extra,
> > +                             const void *extra_data) {}
> > +static inline void __trace_var(u32 event, bool_t cycles, unsigned int extra,
> > +                               const void *extra_data) {}
> > +static inline void __trace_hypercall(uint32_t event, unsigned long op,
> > +                                     const xen_ulong_t *args) {}
> > +#endif
> We try to do away with u32 and friends, so please don't introduce
> new uses - even less so when in one case here you already use
> uint32_t. Similarly please use "bool" in favor of "bool_t".
Roger that.
> Jan
>
>
> .
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel