linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nam Cao <namcao@linutronix.de>
To: Gabriele Monaco <gmonaco@redhat.com>
Cc: linux-kernel@vger.kernel.org,
	Steven Rostedt <rostedt@goodmis.org>,
	linux-trace-kernel@vger.kernel.org,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Tomas Glozar <tglozar@redhat.com>, Juri Lelli <jlelli@redhat.com>,
	Clark Williams <williams@redhat.com>,
	John Kacur <jkacur@redhat.com>
Subject: Re: [PATCH v3 11/17] rv: Retry when da monitor detects race conditions
Date: Tue, 15 Jul 2025 17:23:22 +0200	[thread overview]
Message-ID: <20250715152322.Os4lDq_B@linutronix.de> (raw)
In-Reply-To: <20250715071434.22508-12-gmonaco@redhat.com>

On Tue, Jul 15, 2025 at 09:14:28AM +0200, Gabriele Monaco wrote:
>  static inline bool										\
>  da_event_##name(struct da_monitor *da_mon, enum events_##name event)				\
>  {												\
> -	type curr_state = da_monitor_curr_state_##name(da_mon);					\
> -	type next_state = model_get_next_state_##name(curr_state, event);			\
> -												\
> -	if (next_state != INVALID_STATE) {							\
> -		da_monitor_set_state_##name(da_mon, next_state);				\
> -												\
> -		trace_event_##name(model_get_state_name_##name(curr_state),			\
> -				   model_get_event_name_##name(event),				\
> -				   model_get_state_name_##name(next_state),			\
> -				   model_is_final_state_##name(next_state));			\
> -												\
> -		return true;									\
> +	enum states_##name curr_state, next_state;						\
> +												\
> +	curr_state = READ_ONCE(da_mon->curr_state);						\
> +	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++) {					\
> +		next_state = model_get_next_state_##name(curr_state, event);			\
> +		if (next_state == INVALID_STATE)						\
> +			goto out_react;								\
> +		if (likely(try_cmpxchg(&da_mon->curr_state, &curr_state, next_state)))		\
> +			goto out_success;							\
>  	}											\
> +	/* Special invalid transition if we run out of retries. */				\
> +	curr_state = INVALID_STATE;								\
>  												\
> +out_react:											\
>  	cond_react_##name(curr_state, event);							\
>  												\
>  	trace_error_##name(model_get_state_name_##name(curr_state),				\
>  			   model_get_event_name_##name(event));					\

If I understand correctly, if after 3 tries and we still fail to change the
state, we will invoke the reactor and trace_error? Doesn't that cause a
false positive? Because it is not a violation of the model, it is just a
race making us fail to change the state.

Same below.

Also, I wouldn't use goto unless necessary. Perhaps it is better to put the
code at "out_react:" and "out_success:" into the loop. But that's just my
personal preference, up to you.

Nam

  reply	other threads:[~2025-07-15 15:23 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-15  7:14 [PATCH v3 00/17] rv: Add monitors to validate task switch Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 01/17] tools/rv: Do not skip idle in trace Gabriele Monaco
2025-07-16 11:50   ` Peter Zijlstra
2025-07-16 12:18     ` Gabriele Monaco
2025-07-16 12:41       ` Peter Zijlstra
2025-07-16 13:05         ` Gabriele Monaco
2025-07-16 13:08           ` Peter Zijlstra
2025-07-16 13:13             ` Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 02/17] tools/rv: Stop gracefully also on SIGTERM Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 03/17] rv: Add da_handle_start_run_event_ to per-task monitors Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 04/17] rv: Remove trailing whitespace from tracepoint string Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 05/17] rv: Return init error when registering monitors Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 06/17] rv: Use strings in da monitors tracepoints Gabriele Monaco
2025-07-15 14:11   ` Nam Cao
2025-07-15  7:14 ` [PATCH v3 07/17] rv: Adjust monitor dependencies Gabriele Monaco
2025-07-16  8:19   ` Nam Cao
2025-07-16  8:30     ` Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 08/17] verification/rvgen: Organise Kconfig entries for nested monitors Gabriele Monaco
2025-07-15 14:48   ` Nam Cao
2025-07-16  7:40     ` Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 09/17] tools/dot2c: Fix generated files going over 100 column limit Gabriele Monaco
2025-07-15 15:01   ` Nam Cao
     [not found]     ` <d69862275becf1d296c80a08b29b2081857a85a1.camel@redhat.com>
2025-07-16  9:34       ` Nam Cao
2025-07-15  7:14 ` [PATCH v3 10/17] rv: " Gabriele Monaco
2025-07-15 15:08   ` Nam Cao
2025-07-15 15:24     ` Gabriele Monaco
2025-07-16  8:13       ` Nam Cao
2025-07-16  9:07         ` Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 11/17] rv: Retry when da monitor detects race conditions Gabriele Monaco
2025-07-15 15:23   ` Nam Cao [this message]
2025-07-16  8:20     ` Gabriele Monaco
2025-07-16  8:27       ` Nam Cao
2025-07-16  8:38         ` Gabriele Monaco
2025-07-16  8:45           ` Nam Cao
2025-07-16  8:59             ` Gabriele Monaco
2025-07-16  9:02               ` Nam Cao
2025-07-15  7:14 ` [PATCH v3 12/17] sched: Adapt sched tracepoints for RV task model Gabriele Monaco
2025-07-16 12:38   ` Peter Zijlstra
2025-07-16 13:40     ` Gabriele Monaco
2025-07-16 13:45       ` Peter Zijlstra
2025-07-16 14:07         ` Gabriele Monaco
2025-07-16 14:19           ` Peter Zijlstra
2025-07-16 14:38             ` Gabriele Monaco
2025-07-16 15:31               ` Peter Zijlstra
2025-07-16 16:14                 ` Gabriele Monaco
2025-07-16 15:09     ` Gabriele Monaco
2025-07-16 15:47       ` Peter Zijlstra
2025-07-15  7:14 ` [PATCH v3 13/17] rv: Adapt the sco monitor to the new set_state Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 14/17] rv: Extend snroc model Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 15/17] rv: Replace tss monitor with more complete sts Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 16/17] rv: Add nrp and sssw per-task monitors Gabriele Monaco
2025-07-15  7:14 ` [PATCH v3 17/17] rv: Add opid per-cpu monitor Gabriele Monaco
2025-07-16  9:38   ` Nam Cao
2025-07-16 10:00     ` Gabriele Monaco
2025-07-18 10:26     ` Gabriele Monaco
2025-07-16  9:42 ` [PATCH v3 00/17] rv: Add monitors to validate task switch Nam Cao
2025-07-16 12:20   ` Gabriele Monaco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250715152322.Os4lDq_B@linutronix.de \
    --to=namcao@linutronix.de \
    --cc=gmonaco@redhat.com \
    --cc=jkacur@redhat.com \
    --cc=jlelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglozar@redhat.com \
    --cc=williams@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).