Re: [POC 0/6] Allow SpinLockIrq to use a normal Guard interface

[Boqun Feng]

On Fri, Oct 18, 2024, at 3:22 AM, Andreas Hindborg wrote:
> Boqun Feng <boqun.feng@xxxxxxxxx> writes:
>
>> Hi Thomas,
>>
>> So this series is what I proposed, previously, because the nested
>> interrupt API in C is local_irq_save() and local_irq_restore(), the
>> following Rust code has the problem of enabling interrupt earlier:
>>
>> 	// l1 and l2 are interrupt disabling locks, their guards (i.e.
>> 	// return of lock()) can be used to track interrupt state.
>>
>> 	// interrupts are enabled in the beginning.
>> 	
>> 	let g1 = l1.lock(); // previous interrupt state is enabled.
>> 	let g2 = l2.lock(); // previous interrupt state is disabled.
>>
>> 	drop(g1); // release l1, if we use g1's state, interrupt will be
>> 		  // enabled. But this is obviously wrong. Because g2
>> 		  // can only exist with interrupt disabled.
>>
>> With the new interrupt disable and enable API, instead of a "unsigned
>> long", a percpu variable is used to track the outermost interrupt state
>> and the nested level, so that "drop(g1);" above won't enable interrupts.
>>
>> Although this requires extra cost, but I think it might be worth paying,
>> because this could make Rust's SpinLockIrq simply use a guard interface
>> as SpinLock.
>>
>> Of course, looking for any comments and suggestions.
>
> I am curious what kind of performance impact we would have for this
> counter in hot paths? If it is significant, and if we can design an API
> based on scopes and closures that perform better, we should probably do
> that.
>

We sort of still have that: for example, in your timer example,  because we know
the interrupt is disabled in a timer callback (when it’s executed in hardirq context),
we can do:

    let irq = unsafe { InterruptDisabled::assume_interrupt_disabled() };

    let guard = this.flag.lock_with(irq);

This will save us one unnecessary interrupt disable.

Thanks for trying this out!

Regards,
Boqun

> Best regards,
> Andreas