From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755409AbcGHOwP (ORCPT ); Fri, 8 Jul 2016 10:52:15 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:40928 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754863AbcGHOv7 (ORCPT ); Fri, 8 Jul 2016 10:51:59 -0400 Date: Fri, 8 Jul 2016 16:51:53 +0200 From: Peter Zijlstra To: Steven Rostedt Cc: LKML , Ingo Molnar , Thomas Gleixner , Clark Williams , Andrew Morton Subject: Re: [PATCH] sched/rt: Avoid sending an IPI to a CPU already doing a push Message-ID: <20160708145153.GX30921@twins.programming.kicks-ass.net> References: <20160624112613.4e98c657@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160624112613.4e98c657@gandalf.local.home> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jun 24, 2016 at 11:26:13AM -0400, Steven Rostedt wrote: > The IPI code now needs to call push_tasks() instead of just push_task() as > it will not be receiving an IPI for each CPU that is requesting a PULL. My brain just skidded on that, can you try again with a few more words? > diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c > index d5690b722691..165bcfdbd94b 100644 > --- a/kernel/sched/rt.c > +++ b/kernel/sched/rt.c > @@ -100,6 +100,7 @@ void init_rt_rq(struct rt_rq *rt_rq) > rt_rq->push_flags = 0; > rt_rq->push_cpu = nr_cpu_ids; > raw_spin_lock_init(&rt_rq->push_lock); > + atomic_set(&rt_rq->ipi_count, 0); > init_irq_work(&rt_rq->push_work, push_irq_work_func); > #endif > #endif /* CONFIG_SMP */ > @@ -1917,6 +1918,10 @@ static int find_next_push_cpu(struct rq *rq) > break; > next_rq = cpu_rq(cpu); > > + /* If pushing was already started, ignore */ > + if (atomic_read(&next_rq->rt.ipi_count)) > + continue; > + > /* Make sure the next rq can push to this rq */ > if (next_rq->rt.highest_prio.next < rq->rt.highest_prio.curr) > break; > @@ -1955,6 +1960,7 @@ static void tell_cpu_to_push(struct rq *rq) > return; > > rq->rt.push_flags = RT_PUSH_IPI_EXECUTING; > + atomic_inc(&cpu_rq(cpu)->rt.ipi_count); > > irq_work_queue_on(&rq->rt.push_work, cpu); > } > @@ -1974,11 +1980,12 @@ static void try_to_push_tasks(void *arg) > > rq = cpu_rq(this_cpu); > src_rq = rq_of_rt_rq(rt_rq); > + WARN_ON_ONCE(!atomic_read(&rq->rt.ipi_count)); > > again: > if (has_pushable_tasks(rq)) { > raw_spin_lock(&rq->lock); > - push_rt_task(rq); > + push_rt_tasks(rq); Maybe as a comment around here? > raw_spin_unlock(&rq->lock); > } > > @@ -2000,7 +2007,7 @@ again: > raw_spin_unlock(&rt_rq->push_lock); > > if (cpu >= nr_cpu_ids) > - return; > + goto out; > > /* > * It is possible that a restart caused this CPU to be > @@ -2011,7 +2018,10 @@ again: > goto again; > > /* Try the next RT overloaded CPU */ > + atomic_inc(&cpu_rq(cpu)->rt.ipi_count); > irq_work_queue_on(&rt_rq->push_work, cpu); > +out: > + atomic_dec(&rq->rt.ipi_count); > } I have a vague feeling we're duplicating state, but I can't seem to spot it, maybe I'm wrong. Looks about right, but could use a comment, this stuff is getting rather subtle.