Re: [Xen-devel] [PATCH v2] xen/sched: rework and rename vcpu_force_reschedule()

From: Dario Faggioli <dfaggioli@suse.com>
To: Juergen Gross <jgross@suse.com>, xen-devel@lists.xenproject.org
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Tim Deegan" <tim@xen.org>, "Julien Grall" <julien.grall@arm.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2] xen/sched: rework and rename vcpu_force_reschedule()
Date: Tue, 17 Sep 2019 16:33:05 +0200	[thread overview]
Message-ID: <04ddefb4e66b349135631bd6dd63f3091d0707ec.camel@suse.com> (raw)
In-Reply-To: <20190914064217.4877-1-jgross@suse.com>

[-- Attachment #1.1: Type: text/plain, Size: 2426 bytes --]

On Sat, 2019-09-14 at 08:42 +0200, Juergen Gross wrote:
> vcpu_force_reschedule() is only used for modifying the periodic timer
> of a vcpu. Forcing a vcpu to give up the physical cpu for that
> purpose
> is kind of brutal.
> 
> So instead of doing the reschedule dance just operate on the timer
> directly. By protecting periodic timer modifications against
> concurrent
> timer activation via a per-vcpu lock it is even no longer required to
> bother the target vcpu at all for updating its timer.
> 
> Rename the function to vcpu_set_periodic_timer() as this now reflects
> the functionality.
> 
Personally, I'm rather happy to see the code which was doing that very
weird "let's go through rescheduling" dance going away. I, FWIW, never
understood why periodic timer handling was implemented that way (and
looking back at relevant changelogs does not help).

The code, as it results after applying this patch, is a lot better, and
easier to understand.

Performance and scalability wise, I don't have benchmarks for this
specific patch (but the ones I did included it, as it back then was
part of the core-scheduling series), but I agree with Juergen. I.e., I
think the patch is either neutral or, if it does something, it improves
things.

Furthermore, periodic timer is *not* used any longer (and since quite
some time/kernel versions). Basically, all we do with the periodic
timer is to disable it during boot. At least for Linux, but I think
this is the case for FreeBSD too. So, even if the patch would have a
negative impact (which again I don't think it's the case), we probably
won't see them.

On this grounds (and, of course, on the one that I've looked at the
code, and think it's correct), for the scheduling part:

> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Then, if some words about the outcome of the discussion in this thread,
e.g., a mention to the fact that the old code wasn't really lockless,
and that the new code is a lot more straightforward, it'd be even
better.

But my Rev-by stands, with or without this.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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