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

[chenbaodong <chenbaodong@xxxxxxxxxx>]

On 5/31/19 18:39, Dario Faggioli wrote:
> On Fri, 2019-05-31 at 09:58 +0800, Baodong Chen wrote:
> > keyhandler mainly used for debug usage by developers which can dump
> > xen module(eg. timer, scheduler) status at runtime by input
> > character from console.
> >
> > Signed-off-by: Baodong Chen <chenbaodong@xxxxxxxxxx>
> > ---
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -368,4 +368,13 @@ config DOM0_MEM
> >   
> >   	  Leave empty if you are not sure what to specify.
> >   
> > +config HAS_KEYHANDLER
> > +       bool "Enable/Disable keyhandler"
> > +       default y
> > +       ---help---
> > +         Enable or disable keyhandler function.
> > +         keyhandler mainly used for debug usage by developers which
> > can dump
> > +         xen module(eg. timer, scheduler) status at runtime by input
> > character
> > +         from console.
> > +
> >   endmenu
> I personally like the idea.
>
> I've probably would have called the option CONFIG_KEYHANDLERS, even if
> I can see that we have quite a few CONFIG_HAS_*.
>
> But it's not for me to ask/decide, and I don't have a too strong
> opinion on this anyway, so let's hear what others think.
>
> I'd at least add the 'S', though (as in CONFIG_HAS_KEYHANDLERS).
Yes, can be fixed.
> > --- a/xen/common/cpupool.c
> > +++ b/xen/common/cpupool.c
> > @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct
> > xen_sysctl_cpupool_op *op)
> >       return ret;
> >   }
> >   
> > +#ifdef CONFIG_HAS_KEYHANDLER
> >   void dump_runq(unsigned char key)
> >   {
> >       unsigned long    flags;
> > @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
> >       local_irq_restore(flags);
> >       spin_unlock(&cpupool_lock);
> >   }
> > +#endif
> >   
> >   static int cpu_callback(
> >       struct notifier_block *nfb, unsigned long action, void *hcpu)
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
> >       xfree(sched);
> >   }
> >   
> > +#ifdef CONFIG_HAS_KEYHANDLER
> >   void schedule_dump(struct cpupool *c)
> >   {
> >       unsigned int      i;
> > @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
> >               SCHED_OP(sched, dump_cpu_state, i);
> >       }
> >   }
> > +#endif
> >   
> >   void sched_tick_suspend(void)
> >   {
> Mmm... a lot of #ifdef-fery spread around quite a bit.. I have to admit
> I don't especially like that.
Me too, can leave it as what is was.

but since schedule_dump prototype have external linkage.

so even no one will call it, it maybe still in output executable file, 
right?
> > --- a/xen/include/xen/keyhandler.h
> > +++ b/xen/include/xen/keyhandler.h
> > @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
> >   /* Inject a keypress into the key-handling subsystem. */
> >   extern void handle_keypress(unsigned char key, struct cpu_user_regs
> > *regs);
> >   
> > +#else
> > +static inline void initialize_keytable(void) {}
> > +static inline void register_keyhandler(unsigned char key,
> > keyhandler_fn_t *fn,
> > +                                       const char *desc, bool_t
> > diagnostic) {}
> > +static inline void register_irq_keyhandler(unsigned char key,
> > +                                           irq_keyhandler_fn_t *fn,
> > +                                           const char *desc,
> > +                                           bool_t diagnostic) {}
> > +
> > +static inline void handle_keypress(unsigned char key,
> > +                                   struct cpu_user_regs *regs) {}
> So, with this last change, we have:
>
> static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
>
> But since all keypress_action() does is calling handle_keypress(),
> which is becoming a nop... can't we kill the tasklet alltogether?
the whole keyhandler.c will not compiled when config disabled.

am i misunderstood something?

> > --- a/xen/include/xen/lib.h
> > +++ b/xen/include/xen/lib.h
> > @@ -171,8 +171,10 @@ extern unsigned int tainted;
> >   extern char *print_tainted(char *str);
> >   extern void add_taint(unsigned int taint);
> >   
> > +#ifdef CONFIG_HAS_KEYHANDLER
> >   struct cpu_user_regs;
> >   void dump_execstate(struct cpu_user_regs *);
> > +#endif
> Yes. Or, you provide an empty stub of dump_execstate(), if
> CONFIG_HAS_KEYHANDLER is not defined, which means we don't have to mess
> with #ifdef-s at the caller site(s).
>
> Of course, I'm not maintainer of this specific piece of code, but I'd
> prefer this stub-based approach to be used in general.... ... ...
Agree,  can be fixed.
>   
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int
> > poolid);
> >   void cpupool_rm_domain(struct domain *d);
> >   int cpupool_move_domain(struct domain *d, struct cpupool *c);
> >   int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
> > +#ifdef CONFIG_HAS_KEYHANDLER
> >   void schedule_dump(struct cpupool *c);
> >   extern void dump_runq(unsigned char key);
> > +#endif
> >   
> >   void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
> >   
> ... ... ... Like, for instance, in here.
>
> But again, sine these changes are spread around many files, let's see
> what others prefer, and use the same strategy everywhere.
>
> Regards
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel