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 <[email protected]>
> ---
> Cc: Petr Mladek <[email protected]>
> Cc: John Ogness <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> ---
> 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, ...)
> {
> }
>
