Re: [PATCH v3 1/3] rust: Introduce irq module

[Boqun Feng <boqun.feng@gmail.com>]

On Thu, Aug 15, 2024 at 06:40:28AM +0000, Benno Lossin wrote:
[...]
> >>>>
> >>>> I haven't found a problem with `&IrqDisabled` as the closure parameter,
> >>>> but I may miss something.
> >>>
> >>> We could also use `&'a IrqDisabled` instead of `IrqDisabled<'a>` (note
> >>> the first one doesn't have a lifetime). But there is no behavioral
> >>> difference between the two. Originally the intended API was to use `&'a
> >>> IrqDisabled<'a>` as the closure parameter and `IrqDisabled<'a>` in
> >>> functions that require irqs being disabled. As long as we decide on a
> >>> consistent type, I don't mind either (since then we can avoid
> >>> reborrowing).
> >>>
> >>>> So the key ask from me is: it looks like we are on the same page that
> >>>> when `cb` returns, the IRQ should be in the same disabled state as when
> >>>> it gets called. So how do we express this "requirement" then? Type
> >>>> sytem, comments, safety comments?
> >>>
> >>> I don't think that expressing this in the type system makes sense, since
> >>> the type that we select (`&'a IrqDisabled` or `IrqDisabled<'a>`) will be
> >>> `Copy`. And thus you can just produce as many of those as you want.
> >>>
> > 
> > Hmm.. on a second thought, `Copy` doesn't affect what I'm proposing
> > here, yes one could have as many `IrqDisabled<'a>` as one wants, but
> > making `cb` returns a `(IrqDisabled<'a>, T)` means the `cb` has to prove
> > at least one of the `IrqDisabled<'a>` exists, i.e. it must prove the irq
> > is still disabled, which the requirement of `with_irqs_disabled`, right?
> 
> Yes, but that doesn't do anything. If the token is `Copy`, then we are
> not allowed to have the following API:
> 
>     fn enable_irq(irq: IrqDisabled<'_>);
> 
> Since if the token is `Copy`, you can just copy it, call the function
> and still return an `IrqDisabled<'a>` to satisfy the closure. It only
> adds verbosity IMO.
> 

OK, so I think I'm more clear on this, basically, we are all on the same
page that `cb` of `with_irqs_disabled()` should have the same irq
disable state before and after the call. And my proposal of putting this
into type system seems not worthwhile. However, I think that aligns with
something else I also want to propose: users should be allowed to change
the interrupt state inside `cb`, as long as 1) the state is recovered at
last and 2) not other soundness or invalid context issues. Basically, we
give the users as much freedom as possible. 

So two things I want to make it clear in the document of
`with_irqs_diabled()`:

1.	Users need to make sure the irq state remains the same when `cb`
	returns.
2.	It's up to the users whether the irq is entirely disabled inside
	`cb`, but users have to do it correctly.

Thoughts? Lyude, I think #2 is different than what you have in mind, but
we actually make have users for this. Thoughts?

FYI the following is not uncommon in kernel:

	local_irq_save(flags);
	while (todo) {
		todo = do_sth();

		if (too_long) {
			local_irq_restore(flags);
			if (!irqs_disabled())
				sleep();
			local_irq_save(flags);
		}
	}
	local_irq_restore(flags);

(of course, usually it makes more sense with local_irq_disable() and
local_irq_enable() here).

Regards,
Boqun

> > Or you're saying there could exist an `IrqDisabled<'a>` but the
> > interrupts are enabled?
> 
> No.
> 
> ---
> Cheers,
> Benno
>