Re: [PATCH v2] um: time-travel: fix time corruption

From: Johannes Berg <johannes@sipsolutions.net>
To: Vincent Whitchurch <Vincent.Whitchurch@axis.com>,
	 "linux-um@lists.infradead.org" <linux-um@lists.infradead.org>
Subject: Re: [PATCH v2] um: time-travel: fix time corruption
Date: Thu, 26 Oct 2023 11:10:03 +0200	[thread overview]
Message-ID: <a2a6c524ddb897d3c3b640d0f7262b506e51d983.camel@sipsolutions.net> (raw)
In-Reply-To: <04afd005c5566fcc6a46549afd7b9136a7cec7a9.camel@axis.com>

On Thu, 2023-10-26 at 08:49 +0000, Vincent Whitchurch wrote:
> On Thu, 2023-10-26 at 09:38 +0200, Johannes Berg wrote:
> > 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?
> 
> I tried removing the !irqs_disabled() check and that blew up pretty
> quickly (below) when running the full roadtest suite.  It works fine
> with your unmodified patch so no need to change the comment.
> 

Hah, OK. So maybe when/if I remember what happens there or can figure it
out again, I can update the comment :)

johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um