Re: [PATCH v2 00/14] hrtimer Rust API
From: Dirk Behme
Date: Mon Oct 14 2024 - 02:58:16 EST
On 13.10.24 23:06, Boqun Feng wrote:
On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
On 13.10.24 00:26, Boqun Feng wrote:
On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
On 12.10.24 09:41, Boqun Feng wrote:
On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
On 12.10.24 01:21, Boqun Feng wrote:
On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
Hi Andreas,
Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
Dirk, thanks for reporting!
:)
Boqun Feng <boqun.feng@xxxxxxxxx> writes:
On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
On 18.09.2024 00:27, Andreas Hindborg wrote:
Hi!
This series adds support for using the `hrtimer` subsystem from Rust code.
I tried breaking up the code in some smaller patches, hopefully that will
ease the review process a bit.
Just fyi, having all 14 patches applied I get [1] on the first (doctest)
Example from hrtimer.rs.
This is from lockdep:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
Having just a quick look I'm not sure what the root cause is. Maybe mutex in
interrupt context? Or a more subtle one?
I think it's calling mutex inside an interrupt context as shown by the
callstack:
] __mutex_lock+0xa0/0xa4
] ...
] hrtimer_interrupt+0x1d4/0x2ac
, it is because:
+//! struct ArcIntrusiveTimer {
+//! #[pin]
+//! timer: Timer<Self>,
+//! #[pin]
+//! flag: Mutex<bool>,
+//! #[pin]
+//! cond: CondVar,
+//! }
has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
irq-off is needed for the lock, because otherwise we will hit a self
deadlock due to interrupts:
spin_lock(&a);
> timer interrupt
spin_lock(&a);
Also notice that the IrqDisabled<'_> token can be simply created by
::new(), because irq contexts should guarantee interrupt disabled (i.e.
we don't support nested interrupts*).
I updated the example based on the work in [1]. I think we need to
update `CondVar::wait` to support waiting with irq disabled.
Yes, I agree. This answers one of the open questions I had in the discussion
with Boqun :)
What do you think regarding the other open question: In this *special* case
here, what do you think to go *without* any lock? I mean the 'while *guard
!= 5' loop in the main thread is read only regarding guard. So it doesn't
matter if it *reads* the old or the new value. And the read/modify/write of
guard in the callback is done with interrupts disabled anyhow as it runs in
interrupt context. And with this can't be interrupted (excluding nested
interrupts). So this modification of guard doesn't need to be protected from
being interrupted by a lock if there is no modifcation of guard "outside"
the interupt locked context.
What do you think?
Reading while there is another CPU is writing is data-race, which is UB.
Could you help to understand where exactly you see UB in Andreas' 'while
*guard != 5' loop in case no locking is used? As mentioned I'm under the
Sure, but could you provide the code of what you mean exactly, if you
don't use a lock here, you cannot have a guard. I need to the exact code
to point out where the compiler may "mis-compile" (a result of being
[...]
I thought we are talking about anything like
#[pin_data]
struct ArcIntrusiveTimer {
#[pin]
timer: Timer<Self>,
#[pin]
- flag: SpinLockIrq<u64>,
+ flag: u64,
#[pin]
cond: CondVar,
}
?
Yes, but have you tried to actually use that for the example from
Andreas? I think you will find that you cannot write to `flag` inside
the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
not mutable reference for `ArcIntrusiveTimer`. You can of course use
unsafe to create a mutable reference to `flag`, but it won't be sound,
since you are getting a mutable reference from an immutable reference.
Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
topic we discuss here? I mean we are talking about getting the compiler to
What do you mean? If the code is unsound, you won't want to use it in an
example, right?
Yes, sure. But ;)
In a first step I just wanted to answer the question if we do need a
lock at all in this special example. And that we could do even with
unsound read/modify/write I would guess. And then, in a second step,
if the answer would be "we don't need the lock", then we could think
about how to make the flag handling sound. So I'm talking just about
answering that question, not about the final example code. Step by step :)
read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
should be independent on the result what we want to look at regarding the
locking requirements of 'flag'?
Anyhow, my root motivation was to simplify Andreas example to not use a lock
where not strictly required. And with this make Andreas example independent
Well, if you don't want to use a lock then you need to use atomics,
otherwise it's likely a UB,
And here we are back to the initial question :) Why would it be UB
without lock (and atomics)?
Some (pseudo) assembly:
Lets start with the main thread:
ldr x1, [x0]
<work with x1>
x0 and x1 are registers. x0 contains the address of flag in the main
memory. I.e. that instruction reads (ldr == load) the content of that
memory location (flag) into x1. x1 then contains flag which can be
used then. This is what I mean with "the main thread is read only". If
flag, i.e. x1, does contain the old or new flag value doesn't matter.
I.e. for the read only operation it doesn't matter if it is protected
by a lock as the load (ldr) can't be interrupted.
Now to the TimerCallback:
ldr x1, [x0]
add x1, x1, #1
str x1, [x0]
This is what I mean with read/modify/write. And this needs to be
ensured that it is not interruptable. I.e. that we are scheduled
between ldr and add or between add and str. Yes, I *totally* agree
that for this a lock is needed:
<lock>
ldr x1, [x0]
add x1, x1, #1
str x1, [x0]
<unlock>
But:
In this this special example we know that we are executing this code
in interrupt context. I.e.:
<interrupts are disabled>
ldr x1, [x0]
add x1, x1, #1
str x1, [x0]
<interrupts are still disabled>
So this read/modify/write can't be interrupted because the interrupts
are off. I.e. the interrupt off prevents the scheduling here. And in
this sense replaces the lock. And as mentioned, which value is read by
the main thread doesn't matter.
To summarize: I totally agree that usually a lock would be needed. But
in this special case with (a) read/modify/write in interrupt context
*and* (b) read only in main thread I'm unclear.
So with this back to the main question: What is my misunderstanding
here? I.e. what is UB in this special case? :)
Best regards
Dirk
but atomics are still WIP, so that why I
suggested Andreas to use a lock first. But I guess I didn't realise the
lock needs to be irq-safe when I suggested that.
Regards,
Boqun
on mutex lockdep issues, SpinLockIrq changes and possible required CondVar
updates. But maybe we find an other way to simplify it and decrease the
dependencies. In the end its just example code ;)
Best regards
Dirk
Regards,
Boqun
[...]