From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1427879AbcBSPv4 (ORCPT ); Fri, 19 Feb 2016 10:51:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54008 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1426129AbcBSPvz (ORCPT ); Fri, 19 Feb 2016 10:51:55 -0500 Date: Fri, 19 Feb 2016 16:51:51 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Yuki Shibuya Subject: Re: [PATCH v2 02/14] KVM: x86: simplify atomics in kvm_pit_ack_irq Message-ID: <20160219155150.GC2456@potion.brq.redhat.com> References: <1455736496-374-1-git-send-email-rkrcmar@redhat.com> <1455736496-374-3-git-send-email-rkrcmar@redhat.com> <56C607BB.3000103@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <56C607BB.3000103@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2016-02-18 19:04+0100, Paolo Bonzini: > On 17/02/2016 20:14, Radim Krčmář wrote: >> - value = atomic_dec_return(&ps->pending); >> - if (value < 0) >> - /* spurious acks can be generated if, for example, the >> - * PIC is being reset. Handle it gracefully here >> - */ >> - atomic_inc(&ps->pending); >> - else if (value > 0 && ps->reinject) >> - /* in this case, we had multiple outstanding pit interrupts >> - * that we needed to inject. Reinject >> - */ >> + if (atomic_dec_if_positive(&ps->pending) > 0 && ps->reinject) >> queue_kthread_work(&ps->pit->worker, &ps->pit->expired); > > Here it would have made sense to do already > > if (!ps->reinject) { > WARN_ON_ONCE(ps->pending || !ps->irq_ack); > return; > } I will add the WARN_ON when removing discard notifiers. > spin_lock(...) > if (atomic_dec_if_positive(&ps->pending) > 0) > queue_kthread_work(...); > ps->irq_ack = 1; > spin_unlock(...) > > because ps->pending is only ever nonzero, and irq_ack is only ever zero, > if ps->reinject. (Well, userspace can switch between policies at runtime.) > Not a big deal since the ack notifier is going to > disappear altogether for the discard policy, but the nice thing is that > it lets you remove the ack notifier earlier and disentangle a bit more > discard mode. > > So if you want for v3 you can reorder the patches like this: The end result is going to be identical. I had a version that did something similar and it was pretty tangled as well -- I wanted to remove useless locks before re-using one for the ioctls. (We need the protection earlier, because userspace can control notifiers while PIT is still being initialized. And removing the lock had dependencies.) > > - patch 1, same > > - patch 2, what is outlined above > > - patch 3, remove ack notifier for discard I agree that current ordering looks weird. The dependency tree looked like this in my mind: -[1/14] ,-[2/14] -[4/14] ,-[3/14] | ,-[5/14] | ,-[6/14] +-[7/14] -[8/14] -[9-14/14] I added [2-4/14] early (and a bit out of order), because it made diffs shorter. Dependency on [7/14] can dropped with correct mutexing inside initialization, so the v3 order would be: -[1/14] ,-[3/14] -[8/14] ,-[2/14] -[4/14] ,-[5/14] ,-[6/14] -[7/14] -[9-14/14] With [8/14] (remove ack notifier for discard) as third. Would that be ok? > - patch 4..14 the rest > > Paolo