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 <[email protected]>
---
--- 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
