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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- 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.

> --- 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.

> @@ -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".

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel