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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <3f1baf35-bf0c-47a7-840f-c81974334785@app.fastmail.com>
Date: Fri, 18 Oct 2024 05:42:50 -0700
From: "Boqun Feng" <boqun.feng@...il.com>
To: "Andreas Hindborg" <a.hindborg@...nel.org>
Cc: "Thomas Gleixner" <tglx@...utronix.de>,
 "Dirk Behme" <dirk.behme@...il.com>, "Lyude Paul" <lyude@...hat.com>,
 rust-for-linux@...r.kernel.org, "Danilo Krummrich" <dakr@...hat.com>,
 airlied@...hat.com, "Ingo Molnar" <mingo@...hat.com>,
 "Will Deacon" <will@...nel.org>, "Waiman Long" <longman@...hat.com>,
 "Peter Zijlstra" <peterz@...radead.org>, linux-kernel@...r.kernel.org,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "Wedson Almeida Filho" <wedsonaf@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...sung.com>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>
Subject: Re: [POC 0/6] Allow SpinLockIrq to use a normal Guard interface



On Fri, Oct 18, 2024, at 3:22 AM, Andreas Hindborg wrote:
> Boqun Feng <boqun.feng@...il.com> 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