On Thu, 2023-10-26 at 07:23 +0000, Vincent Whitchurch wrote: > > @@ -839,9 +863,7 @@ static u64 timer_read(struct clocksource *cs) > > */ > > if (!irqs_disabled() && !in_interrupt() && !in_softirq() && > > !time_travel_ext_waiting) > > - time_travel_update_time(time_travel_time + > > - TIMER_MULTIPLIER, > > - false); > > + time_travel_update_time_rel(TIMER_MULTIPLIER); > > return time_travel_time / TIMER_MULTIPLIER; > > } > > The reason I hesitated with putting the whole of > time_travel_update_time() under local_irq_save() in my attempt was > because I didn't quite understand the reason for the !irqs_disabled() > condition here and the comment just above it about recursion and things > getting messed up. If it's OK to disable interrupts as this patch does, > is the !irqs_disabled() condition valid?
Hmm. I was going to say that's different, because it wants to only prevent us from doing this while we're *already* in IRQ context, and the bug you found is calling timer_read() not in IRQ context, but getting an event queued by the signal. But ... now that I think about it, I have a feeling that this was a workaround for the exact same problem, and I just didn't understand it at the time? I mean, recursing into our own processing is now impossible here after this patch - either we're running normally, or the interrupt cannot hit timer_read() in the middle, same as it cannot hit time_travel_handle_real_alarm() in the middle now. Removing that still seems to work with your test, but it's also not a good test for this, since there are no devices etc. that could have interrupts, not sure how to test it right now? Maybe I'll add a comment there saying this might no longer be needed? johannes _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um
