| Message ID | 90868f1dd49680ec63c961ec8c72ceb64f1af091.1741708239.git.namcao@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RV: Linear temporal logic monitors for RT application | expand |
On Tue, 2025-03-11 at 18:05 +0100, Nam Cao wrote: > Each RV monitor has one static buffer to send to the reactors. If > multiple > errors are detected at the same time, the one buffer could be > overwritten. > > Instead, leave it to the reactors to handle buffering. > > Signed-off-by: Nam Cao <namcao@linutronix.de> > --- > Cc: Petr Mladek <pmladek@suse.com> > Cc: John Ogness <john.ogness@linutronix.de> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > include/linux/printk.h | 1 + > include/linux/rv.h | 11 +++--- > include/rv/da_monitor.h | 61 ++++++------------------------ > -- > kernel/printk/internal.h | 1 - > kernel/trace/rv/reactor_panic.c | 7 +--- > kernel/trace/rv/reactor_printk.c | 8 +++-- > kernel/trace/rv/rv_reactors.c | 2 +- > 7 files changed, 26 insertions(+), 65 deletions(-) > > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > index 510c88bfabd4..c55d45544a16 100644 > --- a/include/rv/da_monitor.h > +++ b/include/rv/da_monitor.h > @@ -16,58 +16,11 @@ > #include <linux/bug.h> > #include <linux/sched.h> > > -#ifdef CONFIG_RV_REACTORS > - > -#define DECLARE_RV_REACTING_HELPERS(name, > type) \ > -static char > REACT_MSG_##name[1024]; \ > - > \ > -static inline char *format_react_msg_##name(type curr_state, type > event) \ > - > { \ > - snprintf(REACT_MSG_##name, > 1024, \ > - "rv: monitor %s does not allow event %s on state > %s\n", \ > - > #name, \ > - > model_get_event_name_##name(event), \ > - > model_get_state_name_##name(curr_state)); \ > - return > REACT_MSG_##name; \ > - > } \ > - > \ > -static void cond_react_##name(char > *msg) \ > - > { \ > - if > (rv_##name.react) \ > - > rv_##name.react(msg); \ > - > } \ > - > \ > -static bool > rv_reacting_on_##name(void) \ > - > { \ > - return > rv_reacting_on(); \ > -} > - > -#else /* CONFIG_RV_REACTOR */ > - > -#define DECLARE_RV_REACTING_HELPERS(name, > type) \ > -static inline char *format_react_msg_##name(type curr_state, type > event) \ > - > { \ > - return > NULL; \ > - > } \ > - > \ > -static void cond_react_##name(char > *msg) \ > - > { \ > - > return; \ > - > } \ > - > \ > -static bool > rv_reacting_on_##name(void) \ > - > { \ > - return > 0; \ > -} > -#endif > - I don't think you need to remove those helper functions, why not just having format_react_msg_ prepare the arguments for react? cond_react might be mildly useful also for ltl, we may think about putting it somewhere else and/or refactoring it a bit to include reacting_on (which is indeed global and doesn't require a per-monitor wrapper). > /* > * Generic helpers for all types of deterministic automata monitors. > */ > #define DECLARE_DA_MON_GENERIC_HELPERS(name, > type) \ > > \ > -DECLARE_RV_REACTING_HELPERS(name, > type) \ > - > \ > /* > \ > * da_monitor_reset_##name - reset a monitor and setting it to init > state \ > > */ \ > @@ -170,8 +123,11 @@ da_event_##name(struct da_monitor *da_mon, enum > events_##name event) \ > return > true; \ > } > \ > > \ > - if > (rv_reacting_on_##name()) \ > - > cond_react_##name(format_react_msg_##name(curr_state, event)); \ > + if (rv_reacting_on() && > rv_##name.react) \ > + rv_##name.react("rv: monitor %s does not allow event > %s on state %s\n", \ > + #name, > \ > + model_get_event_name_##name(event), > \ > + model_get_state_name_##name(curr_sta > te)); \ > > \ > trace_error_##name(model_get_state_name_##name(curr_state), > \ > > model_get_event_name_##name(event)); \ > @@ -202,8 +158,11 @@ static inline bool da_event_##name(struct > da_monitor *da_mon, struct task_struct > return > true; \ > } > \ > > \ > - if > (rv_reacting_on_##name()) \ > - > cond_react_##name(format_react_msg_##name(curr_state, event)); \ > + if (rv_reacting_on() && > rv_##name.react) \ > + rv_##name.react("rv: monitor %s does not allow event > %s on state %s\n", \ > + #name, > \ > + model_get_event_name_##name(event), > \ > + model_get_state_name_##name(curr_sta > te)); \ > > \ > trace_error_##name(tsk- > >pid, \ > > model_get_state_name_##name(curr_state), \ > diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h > index a91bdf802967..28afdeb58412 100644 > --- a/kernel/printk/internal.h > +++ b/kernel/printk/internal.h > @@ -71,7 +71,6 @@ int vprintk_store(int facility, int level, > const char *fmt, va_list args); > > __printf(1, 0) int vprintk_default(const char *fmt, va_list args); > -__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args); > > void __printk_safe_enter(void); > void __printk_safe_exit(void); > diff --git a/kernel/trace/rv/reactor_panic.c > b/kernel/trace/rv/reactor_panic.c > index 0186ff4cbd0b..4addabc9bcf1 100644 > --- a/kernel/trace/rv/reactor_panic.c > +++ b/kernel/trace/rv/reactor_panic.c > @@ -13,15 +13,10 @@ > #include <linux/init.h> > #include <linux/rv.h> > > -static void rv_panic_reaction(char *msg) > -{ > - panic(msg); > -} > - > static struct rv_reactor rv_panic = { > .name = "panic", > .description = "panic the system if an exception is found.", > - .react = rv_panic_reaction > + .react = panic > }; For the sake of verbosity, I would still keep a wrapper function around panic, just to show directly from this file how should a react() function look like, as well as allowing future modifications, if needed. Not that the additional function call would be much of a problem during panic, I believe. Good improvement overall, thanks. Gabriele > > static int __init register_react_panic(void) > diff --git a/kernel/trace/rv/reactor_printk.c > b/kernel/trace/rv/reactor_printk.c > index 178759dbf89f..a15db3fc8b82 100644 > --- a/kernel/trace/rv/reactor_printk.c > +++ b/kernel/trace/rv/reactor_printk.c > @@ -12,9 +12,13 @@ > #include <linux/init.h> > #include <linux/rv.h> > > -static void rv_printk_reaction(char *msg) > +static void rv_printk_reaction(const char *msg, ...) > { > - printk_deferred(msg); > + va_list args; > + > + va_start(args, msg); > + vprintk_deferred(msg, args); > + va_end(args); > } > > static struct rv_reactor rv_printk = { > diff --git a/kernel/trace/rv/rv_reactors.c > b/kernel/trace/rv/rv_reactors.c > index 7b49cbe388d4..885661fe2b6e 100644 > --- a/kernel/trace/rv/rv_reactors.c > +++ b/kernel/trace/rv/rv_reactors.c > @@ -468,7 +468,7 @@ void reactor_cleanup_monitor(struct > rv_monitor_def *mdef) > /* > * Nop reactor register > */ > -static void rv_nop_reaction(char *msg) > +static void rv_nop_reaction(const char *msg, ...) > { > } >
Hi Gabriele, On Wed, Mar 12, 2025 at 08:58:56AM +0100, Gabriele Monaco wrote: > On Tue, 2025-03-11 at 18:05 +0100, Nam Cao wrote: > > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > > index 510c88bfabd4..c55d45544a16 100644 > > --- a/include/rv/da_monitor.h > > +++ b/include/rv/da_monitor.h > > @@ -16,58 +16,11 @@ > > #include <linux/bug.h> > > #include <linux/sched.h> > > > > -#ifdef CONFIG_RV_REACTORS > > - > > -#define DECLARE_RV_REACTING_HELPERS(name, > > type) \ > > -static char > > REACT_MSG_##name[1024]; \ > > - > > \ > > -static inline char *format_react_msg_##name(type curr_state, type > > event) \ > > - > > { \ > > - snprintf(REACT_MSG_##name, > > 1024, \ > > - "rv: monitor %s does not allow event %s on state > > %s\n", \ > > - > > #name, \ > > - > > model_get_event_name_##name(event), \ > > - > > model_get_state_name_##name(curr_state)); \ > > - return > > REACT_MSG_##name; \ > > - > > } \ > > - > > \ > > -static void cond_react_##name(char > > *msg) \ > > - > > { \ > > - if > > (rv_##name.react) \ > > - > > rv_##name.react(msg); \ > > - > > } \ > > - > > \ > > -static bool > > rv_reacting_on_##name(void) \ > > - > > { \ > > - return > > rv_reacting_on(); \ > > -} > > - > > -#else /* CONFIG_RV_REACTOR */ > > - > > -#define DECLARE_RV_REACTING_HELPERS(name, > > type) \ > > -static inline char *format_react_msg_##name(type curr_state, type > > event) \ > > - > > { \ > > - return > > NULL; \ > > - > > } \ > > - > > \ > > -static void cond_react_##name(char > > *msg) \ > > - > > { \ > > - > > return; \ > > - > > } \ > > - > > \ > > -static bool > > rv_reacting_on_##name(void) \ > > - > > { \ > > - return > > 0; \ > > -} > > -#endif > > - > > I don't think you need to remove those helper functions, why not just > having format_react_msg_ prepare the arguments for react? I'm not sure what you mean. Making format_react_msg_* a macro that is preprocessed into arguments? Then make cond_react_*() a variadic function, so that we can "pass" format_react_msg_* to it? Going that way would also need a vreact() variant, as cond_react_*() cannot pass on the variadic arguments to react(). Instead, is it cleaner to do the below? diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h index 510c88bfabd4..e185ebf894a4 100644 --- a/include/rv/da_monitor.h +++ b/include/rv/da_monitor.h @@ -19,22 +19,14 @@ #ifdef CONFIG_RV_REACTORS #define DECLARE_RV_REACTING_HELPERS(name, type) \ -static char REACT_MSG_##name[1024]; \ - \ -static inline char *format_react_msg_##name(type curr_state, type event) \ -{ \ - snprintf(REACT_MSG_##name, 1024, \ - "rv: monitor %s does not allow event %s on state %s\n", \ - #name, \ - model_get_event_name_##name(event), \ - model_get_state_name_##name(curr_state)); \ - return REACT_MSG_##name; \ -} \ - \ -static void cond_react_##name(char *msg) \ +static void cond_react_##name(type curr_state, type event) \ { \ - if (rv_##name.react) \ - rv_##name.react(msg); \ + if (!rv_##name.react) \ + return; \ + rv_##name.react("rv: monitor %s does not allow event %s on state %s\n", \ + #name, \ + model_get_event_name_##name(event), \ + model_get_state_name_##name(curr_state)); \ } \ \ static bool rv_reacting_on_##name(void) \ @@ -45,12 +37,7 @@ static bool rv_reacting_on_##name(void) \ #else /* CONFIG_RV_REACTOR */ #define DECLARE_RV_REACTING_HELPERS(name, type) \ -static inline char *format_react_msg_##name(type curr_state, type event) \ -{ \ - return NULL; \ -} \ - \ -static void cond_react_##name(char *msg) \ +static void cond_react_##name(type curr_state, type event) \ { \ return; \ } \ @@ -171,7 +158,7 @@ da_event_##name(struct da_monitor *da_mon, enum events_##name event) \ } \ \ if (rv_reacting_on_##name()) \ - cond_react_##name(format_react_msg_##name(curr_state, event)); \ + cond_react_##name(curr_state, event); \ \ trace_error_##name(model_get_state_name_##name(curr_state), \ model_get_event_name_##name(event)); \ @@ -203,7 +190,7 @@ static inline bool da_event_##name(struct da_monitor *da_mon, struct task_struct } \ \ if (rv_reacting_on_##name()) \ - cond_react_##name(format_react_msg_##name(curr_state, event)); \ + cond_react_##name(curr_state, event); \ \ trace_error_##name(tsk->pid, \ model_get_state_name_##name(curr_state), \ Best regards, Nam
diff --git a/include/linux/printk.h b/include/linux/printk.h index 4217a9f412b2..11e49b299312 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -154,6 +154,7 @@ int vprintk_emit(int facility, int level, asmlinkage __printf(1, 0) int vprintk(const char *fmt, va_list args); +__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args); asmlinkage __printf(1, 2) __cold int _printk(const char *fmt, ...); diff --git a/include/linux/rv.h b/include/linux/rv.h index 8883b41d88ec..5482651ed020 100644 --- a/include/linux/rv.h +++ b/include/linux/rv.h @@ -38,7 +38,7 @@ union rv_task_monitor { struct rv_reactor { const char *name; const char *description; - void (*react)(char *msg); + void (*react)(const char *msg, ...); }; #endif @@ -49,9 +49,7 @@ struct rv_monitor { int (*enable)(void); void (*disable)(void); void (*reset)(void); -#ifdef CONFIG_RV_REACTORS - void (*react)(char *msg); -#endif + void (*react)(const char *msg, ...); }; bool rv_monitoring_on(void); @@ -64,6 +62,11 @@ void rv_put_task_monitor_slot(int slot); bool rv_reacting_on(void); int rv_unregister_reactor(struct rv_reactor *reactor); int rv_register_reactor(struct rv_reactor *reactor); +#else +static inline bool rv_reacting_on(void) +{ + return false; +} #endif /* CONFIG_RV_REACTORS */ #endif /* CONFIG_RV */ diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h index 510c88bfabd4..c55d45544a16 100644 --- a/include/rv/da_monitor.h +++ b/include/rv/da_monitor.h @@ -16,58 +16,11 @@ #include <linux/bug.h> #include <linux/sched.h> -#ifdef CONFIG_RV_REACTORS - -#define DECLARE_RV_REACTING_HELPERS(name, type) \ -static char REACT_MSG_##name[1024]; \ - \ -static inline char *format_react_msg_##name(type curr_state, type event) \ -{ \ - snprintf(REACT_MSG_##name, 1024, \ - "rv: monitor %s does not allow event %s on state %s\n", \ - #name, \ - model_get_event_name_##name(event), \ - model_get_state_name_##name(curr_state)); \ - return REACT_MSG_##name; \ -} \ - \ -static void cond_react_##name(char *msg) \ -{ \ - if (rv_##name.react) \ - rv_##name.react(msg); \ -} \ - \ -static bool rv_reacting_on_##name(void) \ -{ \ - return rv_reacting_on(); \ -} - -#else /* CONFIG_RV_REACTOR */ - -#define DECLARE_RV_REACTING_HELPERS(name, type) \ -static inline char *format_react_msg_##name(type curr_state, type event) \ -{ \ - return NULL; \ -} \ - \ -static void cond_react_##name(char *msg) \ -{ \ - return; \ -} \ - \ -static bool rv_reacting_on_##name(void) \ -{ \ - return 0; \ -} -#endif - /* * Generic helpers for all types of deterministic automata monitors. */ #define DECLARE_DA_MON_GENERIC_HELPERS(name, type) \ \ -DECLARE_RV_REACTING_HELPERS(name, type) \ - \ /* \ * da_monitor_reset_##name - reset a monitor and setting it to init state \ */ \ @@ -170,8 +123,11 @@ da_event_##name(struct da_monitor *da_mon, enum events_##name event) \ return true; \ } \ \ - if (rv_reacting_on_##name()) \ - cond_react_##name(format_react_msg_##name(curr_state, event)); \ + if (rv_reacting_on() && rv_##name.react) \ + rv_##name.react("rv: monitor %s does not allow event %s on state %s\n", \ + #name, \ + model_get_event_name_##name(event), \ + model_get_state_name_##name(curr_state)); \ \ trace_error_##name(model_get_state_name_##name(curr_state), \ model_get_event_name_##name(event)); \ @@ -202,8 +158,11 @@ static inline bool da_event_##name(struct da_monitor *da_mon, struct task_struct return true; \ } \ \ - if (rv_reacting_on_##name()) \ - cond_react_##name(format_react_msg_##name(curr_state, event)); \ + if (rv_reacting_on() && rv_##name.react) \ + rv_##name.react("rv: monitor %s does not allow event %s on state %s\n", \ + #name, \ + model_get_event_name_##name(event), \ + model_get_state_name_##name(curr_state)); \ \ trace_error_##name(tsk->pid, \ model_get_state_name_##name(curr_state), \ diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h index a91bdf802967..28afdeb58412 100644 --- a/kernel/printk/internal.h +++ b/kernel/printk/internal.h @@ -71,7 +71,6 @@ int vprintk_store(int facility, int level, const char *fmt, va_list args); __printf(1, 0) int vprintk_default(const char *fmt, va_list args); -__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args); void __printk_safe_enter(void); void __printk_safe_exit(void); diff --git a/kernel/trace/rv/reactor_panic.c b/kernel/trace/rv/reactor_panic.c index 0186ff4cbd0b..4addabc9bcf1 100644 --- a/kernel/trace/rv/reactor_panic.c +++ b/kernel/trace/rv/reactor_panic.c @@ -13,15 +13,10 @@ #include <linux/init.h> #include <linux/rv.h> -static void rv_panic_reaction(char *msg) -{ - panic(msg); -} - static struct rv_reactor rv_panic = { .name = "panic", .description = "panic the system if an exception is found.", - .react = rv_panic_reaction + .react = panic }; static int __init register_react_panic(void) diff --git a/kernel/trace/rv/reactor_printk.c b/kernel/trace/rv/reactor_printk.c index 178759dbf89f..a15db3fc8b82 100644 --- a/kernel/trace/rv/reactor_printk.c +++ b/kernel/trace/rv/reactor_printk.c @@ -12,9 +12,13 @@ #include <linux/init.h> #include <linux/rv.h> -static void rv_printk_reaction(char *msg) +static void rv_printk_reaction(const char *msg, ...) { - printk_deferred(msg); + va_list args; + + va_start(args, msg); + vprintk_deferred(msg, args); + va_end(args); } static struct rv_reactor rv_printk = { diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c index 7b49cbe388d4..885661fe2b6e 100644 --- a/kernel/trace/rv/rv_reactors.c +++ b/kernel/trace/rv/rv_reactors.c @@ -468,7 +468,7 @@ void reactor_cleanup_monitor(struct rv_monitor_def *mdef) /* * Nop reactor register */ -static void rv_nop_reaction(char *msg) +static void rv_nop_reaction(const char *msg, ...) { }
Each RV monitor has one static buffer to send to the reactors. If multiple errors are detected at the same time, the one buffer could be overwritten. Instead, leave it to the reactors to handle buffering. Signed-off-by: Nam Cao <namcao@linutronix.de> --- Cc: Petr Mladek <pmladek@suse.com> Cc: John Ogness <john.ogness@linutronix.de> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> --- include/linux/printk.h | 1 + include/linux/rv.h | 11 +++--- include/rv/da_monitor.h | 61 ++++++-------------------------- kernel/printk/internal.h | 1 - kernel/trace/rv/reactor_panic.c | 7 +--- kernel/trace/rv/reactor_printk.c | 8 +++-- kernel/trace/rv/rv_reactors.c | 2 +- 7 files changed, 26 insertions(+), 65 deletions(-)