Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Tue, 2019-04-23 at 11:56 +0200, Juergen Gross wrote:
> On 23/04/2019 11:50, George Dunlap wrote:
> > 
> > > On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@xxxxxxxx>
> > > wrote:
> > > 
> > > We can, therefore, get rid of all the `if`-s testing prev and
> > > next being
> > > different, trading them with an ASSERT() (on ARM, the ASSERT()
> > > was even
> > > already there!)
> > 
> > Keeping in mind that ASSERT() is merely a debugging aid: suppose
> > that testing didn’t discover this, and a bug that violated this
> > assumption slipped into production.  Would the patched code DTRT?
> 
> The unpatched code would already do something wrong: it would clear
> v->is_running (through context_saved()) for the running vcpu,
> potentially leading to all sorts of wrong decisions in the scheduling
> code later.
> 
Exactly! Which basically means it's quite likely that, if we ever get
past the call to:

  return continue_running(prev)  (in schedule.c:schedule())

with next==prev, things will go very wrong, without the `if`-s that
this patch is killing being of much help.

And, yes, this is (micro) optimizing, but it's also an attempt to
improve the code, making it clear, for people paying with
context_switch(), that they don't have to deal with the special case of
prev and next being equal, while right now they may think they do.

Even if we decide to keep the `if`-s, I'd still like to have the ASSERT
() (and a comment explaining why we have both the assertion and the
checks).

I haven't done archaeology to find out when it was (if ever) that it
was ok to call context_switch() with next==prev. I can, and see about
adding what I find in the changelog, if you (George) think it could be
interesting.

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel