Re: [Xen-devel] [PATCH] xen/sched: fix locking in sched_tick_[suspend|resume]()

[Jürgen Groß <jgross@xxxxxxxx>]

On 04.10.19 16:34, George Dunlap wrote:
> On 10/4/19 3:24 PM, Jürgen Groß wrote:
> > On 04.10.19 16:08, George Dunlap wrote:
> > > On 10/4/19 7:40 AM, Juergen Gross wrote:
> > > > sched_tick_suspend() and sched_tick_resume() should not call the
> > > > scheduler specific timer handlers in case the cpu they are running on
> > > > is just being moved to or from a cpupool.
> > > >
> > > > Use a new percpu lock for that purpose.
> > > Is there a reason we can't use the pcpu_schedule_lock() instead of
> > > introducing a new one?  Sorry if this is obvious, but it's been a while
> > > since I poked around this code.
> > Lock contention would be higher especially with credit2 which is using a
> > per-core or even per-socket lock. We don't care about other scheduling
> > activity here, all we need is a guard against our per-cpu scheduler
> > data being changed beneath our feet.
> Is this code really being called so often that we need to worry about
> this level of contention?
Its called each time idle is entered and left again.

Especially with core scheduling there is a high probability of multiple
cpus leaving idle at the same time and the per-scheduler lock being used
in parallel already.

> We already have a *lot* of locks; and in this case you're adding a
> second lock which interacts with the per-scheduler cpu lock.  This just
> seems like asking for trouble.
In which way does it interact with the per-scheduler cpu lock?

> I won't Nack the patch, but I don't think I would ack it without clear
> evidence that the extra lock has a performance improvement that's worth
> the cost of the extra complexity.
I think complexity is lower this way. Especially considering the per-
scheduler lock changing with moving a cpu to or from a cpupool.


Juergen

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