Skip to content

Disable IRQs in OQueue spinlocks.#201

Merged
arthurp merged 1 commit intomainfrom
arthurp/oqueue-disable-irq-max
Mar 31, 2026
Merged

Disable IRQs in OQueue spinlocks.#201
arthurp merged 1 commit intomainfrom
arthurp/oqueue-disable-irq-max

Conversation

@arthurp
Copy link
Copy Markdown
Contributor

@arthurp arthurp commented Mar 30, 2026

This makes them safer to operate on inside IRQ handlers.

This makes them safer to operate on inside IRQ handlers.
@arthurp arthurp marked this pull request as ready for review March 30, 2026 16:53
@arthurp arthurp requested a review from a team as a code owner March 30, 2026 16:53
Copy link
Copy Markdown
Contributor

@ioeddk ioeddk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Additional comment: the SpinLock class says:

/// The guard behavior can be temporarily upgraded from [`PreemptDisabled`] to
/// [`LocalIrqDisabled`] using the [`disable_irq`] method.
///
/// [`disable_irq`]: Self::disable_irq

So if we don't want to disable IRQ in every OQueue, we can keep it preempt disabled only, and disable it ad hoc.

@arthurp
Copy link
Copy Markdown
Contributor Author

arthurp commented Mar 30, 2026

LGTM. Additional comment: the SpinLock class says:

/// The guard behavior can be temporarily upgraded from [`PreemptDisabled`] to
/// [`LocalIrqDisabled`] using the [`disable_irq`] method.
///
/// [`disable_irq`]: Self::disable_irq

So if we don't want to disable IRQ in every OQueue, we can keep it preempt disabled only, and disable it ad hoc.

That would have been the "minimal" case I was thinking about. I think for any given OQueue either no access or all accesses need to disable IRQs, so ad-hoc usage is error prone. Maybe with the new OQueue we need a way to create "IRQ-safe" OQueues which do this disabling and normal ones which panic if accessed from an IRQ context.

I'm not going to merge this at the moment. It's definitely possible it breaks something and I don't want to make main a mine field without good reason. Maybe I'll merge later this week once we have a chance to chat about it.

@arthurp arthurp merged commit 873113a into main Mar 31, 2026
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants