[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.