Re: [RFC PATCH v2 10/12] rv: Retry when da monitor detects race conditions

On Mon, 2025-05-19 at 12:38 +0200, Nam Cao wrote:

On Mon, May 19, 2025 at 12:28:12PM +0200, Gabriele Monaco wrote:

quoted

Mmh, although definitely unlikely, I'm thinking of a case in which
the
event starts on one CPU and at the same time we see events in IRQ
and 
on another CPU, let's say continuously. Nothing forbids that
between
any two consecutive try_cmpxchg another CPU/context changes the
next
state (making the local try_cmpxchg fail).
In practice I've never seen it going on the second iteration, as
the
critical section is really tiny, but I'm not sure we can guarantee
this
never happens.
Or am I missing something?

I have a feeling that you missed my point. I agree that the retrying
is
needed, because we may race with another.

What I am proposing is that we drop the MAX_DA_RETRY_RACING_EVENTS,
and
just keep retrying until we succeed.

And that's safe to do, because the maximum number of retries is the
number
of tasks contending with us to set the monitor's state. So we know we
won't
be retrying for long.

I get this point, what I mean is: can we really guarantee the number of
contending tasks (or contexts) is finite?
In other words, the try_cmpxchg guarantees 1 and only 1 actor wins
every time, but cannot guarantee all actors will eventually win, an
actor /could/ be hanging there forever.

This handler is running for each event in the monitor and tracepoint
handlers can be interrupted as well as run in interrupt context (where
of course they cannot be interrupted). I don't think the number of
actors is bounded by the number of CPUs.
I see this situation is extremely unlikely, but in an exotic scenario
where a CPU is sufficiently slower than others (e.g. in a VM) I believe
we can see this critical section large enough for this to potentially
happen.

I'm not quite afraid of infinite loops, but rather RV introducing
unbounded latency very hard to track and without any reporting.
Chances are, since tracepoints and actual traced events are not atomic,
that by the time this delayed context /wins/ the RV event is no longer
current, so we may see an error already.

Does it make sense to you or am I making it more complex than it should
be?

Thanks,
Gabriele