From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752636AbbE0JJh (ORCPT ); Wed, 27 May 2015 05:09:37 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:43366 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751283AbbE0JJc (ORCPT ); Wed, 27 May 2015 05:09:32 -0400 Date: Wed, 27 May 2015 11:09:22 +0200 From: Peter Zijlstra To: Konstantin Khlebnikov Cc: Ingo Molnar , linux-kernel@vger.kernel.org, x86@kernel.org, Linus Torvalds , Oleg Nesterov , Steven Rostedt Subject: Re: [PATCH RESEND] sched/preempt: fix cond_resched_lock() and cond_resched_softirq() Message-ID: <20150527090922.GW3644@twins.programming.kicks-ass.net> References: <20150514162044.27876.20374.stgit@buzz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150514162044.27876.20374.stgit@buzz> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 14, 2015 at 07:23:27PM +0300, Konstantin Khlebnikov wrote: > These functions check should_resched() before unlocking spinlock/bh-enable: > preempt_count always non-zero => should_resched() always returns false. > cond_resched_lock() works iff spin_needbreak is set. > > This patch adds argument "preempt_offset" to should_resched() add > rearranges preempt_count offset constants for that: > > PREEMPT_OFFSET - offset after preempt_disable() (0 if CONFIG_PREEMPT_COUNT=n) > PREEMPT_LOCK_OFFSET - offset after spin_lock() (alias for PREEMPT_OFFSET) > SOFTIRQ_DISABLE_OFFSET - offset after local_bh_distable() > SOFTIRQ_LOCK_OFFSET - offset after spin_lock_bh() > > Signed-off-by: Konstantin Khlebnikov Sorry, but it doesn't apply anymore because of that whole pagefault_disable() muck we merged. > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 48d3c5d2ecc9..6e73b74c0c60 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -2177,7 +2177,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) > vc->runner = vcpu; > if (n_ceded == vc->n_runnable) { > kvmppc_vcore_blocked(vc); > - } else if (should_resched()) { > + } else if (should_resched(PREEMPT_LOCK_OFFSET)) { I'm thinking this wants to be: need_resched() ? > vc->vcore_state = VCORE_PREEMPT; > /* Let something else run */ > cond_resched_lock(&vc->lock); > diff --git a/include/asm-generic/preempt.h b/include/asm-generic/preempt.h > index eb6f9e6c3075..e91fb799a6da 100644 > --- a/include/asm-generic/preempt.h > +++ b/include/asm-generic/preempt.h > @@ -71,9 +71,9 @@ static __always_inline bool __preempt_count_dec_and_test(void) > /* > * Returns true when we need to resched and can (barring IRQ state). > */ > -static __always_inline bool should_resched(void) > +static __always_inline bool should_resched(int offset) > { > - return unlikely(!preempt_count() && tif_need_resched()); > + return unlikely(preempt_count() == offset && tif_need_resched()); > } > > #ifdef CONFIG_PREEMPT So the reason I held off on this patch for a wee bit is because I don't like the should_resched() change you did; although I fully understand why you did it. That said, I could not come up with anything better either and I suppose that once we fix that ppc-kvm user, there really isn't a user left outside of core code and thus we can deal with a slightly dangerous function. I did not really look, but it would be good if we could also get rid of the Xen usage.