From: christoffer.dall@linaro.org (Christoffer Dall)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
Date: Fri, 3 Jul 2015 23:01:10 +0200 [thread overview]
Message-ID: <20150703210110.GA10878@cbox> (raw)
In-Reply-To: <5596B0D0.3080505@arm.com>
On Fri, Jul 03, 2015 at 04:57:04PM +0100, Andre Przywara wrote:
> Hi Christoffer,
>
> ....
>
> >> +
> >> +static struct its_collection *vits_new_collection(struct kvm *kvm, u32 coll_id)
> >> +{
> >> + struct its_collection *collection;
> >> +
> >> + collection = kmalloc(sizeof(struct its_collection), GFP_KERNEL);
> >
> > If I manage to understand the structure here, you're calling all these
> > handler functions with a spinlock held so any operation that may sleep
> > could cause your system to deadlock.
> >
> > I'll stop looking for these and recommend you go over the whole series
> > for these.
>
> Do you reckon it would be sufficient to just avoid the kmallocs inside
> the lock? For this case above we could go with some storage space
> preallocated outside of the lock (I hope).
>
Yes, you can preallocate or you need to grab a mutex instead of a
spinlock...
> >
> > Perhaps the right thing to do is to synchronize access to your data
> > structure (why you hold the spinlock right?) with RCU if you can...
>
> Well, the point is that there is not one ITS data structure, but it's a
> mesh of lists connected to each other. I'd like to avoid going down with
> RCU there, so I'd like to keep it all under one lock.
> I wonder if this could be mutex_lock_interruptible, though. From the top
> of your head, is there anything that would prevent that? I reckon ITS
> access contention is rather rare (though possible), so a sleeping VCPU
> wouldn't harm us so much in practice, would it?
>
We know from experience from x86 that one of the things they had to look
at was to get the run-loop lock-free, and we went through a lot of
effort to do that on ARM too. Along came the vgic and that was out the
window, and now it feels like we're just grabbing spinlocks all over the
place.
I'm fine with a mutex if other solutions are not easy/possible and it's
in a truly non-critical path, but I wouldn't to speculate about the
level of contention at this point without profiling something.
In any case, let's fix the potential host-kernel deadlock issues in the
most elegant way first and let's try to think about not grabbing too
many spinlocks in this code and take it from there.
Thanks,
-Christoffer
WARNING: multiple messages have this Message-ID (diff)
From: Christoffer Dall <christoffer.dall@linaro.org>
To: Andre Przywara <andre.przywara@arm.com>
Cc: marc.zyngier@arm.com, kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
Date: Fri, 3 Jul 2015 23:01:10 +0200 [thread overview]
Message-ID: <20150703210110.GA10878@cbox> (raw)
In-Reply-To: <5596B0D0.3080505@arm.com>
On Fri, Jul 03, 2015 at 04:57:04PM +0100, Andre Przywara wrote:
> Hi Christoffer,
>
> ....
>
> >> +
> >> +static struct its_collection *vits_new_collection(struct kvm *kvm, u32 coll_id)
> >> +{
> >> + struct its_collection *collection;
> >> +
> >> + collection = kmalloc(sizeof(struct its_collection), GFP_KERNEL);
> >
> > If I manage to understand the structure here, you're calling all these
> > handler functions with a spinlock held so any operation that may sleep
> > could cause your system to deadlock.
> >
> > I'll stop looking for these and recommend you go over the whole series
> > for these.
>
> Do you reckon it would be sufficient to just avoid the kmallocs inside
> the lock? For this case above we could go with some storage space
> preallocated outside of the lock (I hope).
>
Yes, you can preallocate or you need to grab a mutex instead of a
spinlock...
> >
> > Perhaps the right thing to do is to synchronize access to your data
> > structure (why you hold the spinlock right?) with RCU if you can...
>
> Well, the point is that there is not one ITS data structure, but it's a
> mesh of lists connected to each other. I'd like to avoid going down with
> RCU there, so I'd like to keep it all under one lock.
> I wonder if this could be mutex_lock_interruptible, though. From the top
> of your head, is there anything that would prevent that? I reckon ITS
> access contention is rather rare (though possible), so a sleeping VCPU
> wouldn't harm us so much in practice, would it?
>
We know from experience from x86 that one of the things they had to look
at was to get the run-loop lock-free, and we went through a lot of
effort to do that on ARM too. Along came the vgic and that was out the
window, and now it feels like we're just grabbing spinlocks all over the
place.
I'm fine with a mutex if other solutions are not easy/possible and it's
in a truly non-critical path, but I wouldn't to speculate about the
level of contention at this point without profiling something.
In any case, let's fix the potential host-kernel deadlock issues in the
most elegant way first and let's try to think about not grabbing too
many spinlocks in this code and take it from there.
Thanks,
-Christoffer
next prev parent reply other threads:[~2015-07-03 21:01 UTC|newest]
Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-29 9:53 [PATCH 00/13] arm64: KVM: GICv3 ITS emulation Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-05-29 9:53 ` [PATCH 01/13] KVM: arm/arm64: VGIC: don't track used LRs in the distributor Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-12 17:23 ` Eric Auger
2015-05-29 9:53 ` [PATCH 02/13] KVM: extend struct kvm_msi to hold a 32-bit device ID Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 8:49 ` Eric Auger
2015-06-09 8:49 ` Eric Auger
2015-06-28 19:12 ` Christoffer Dall
2015-06-28 19:12 ` Christoffer Dall
2015-06-29 14:53 ` Andre Przywara
2015-06-29 14:53 ` Andre Przywara
2015-06-29 15:02 ` Christoffer Dall
2015-06-29 15:02 ` Christoffer Dall
2015-05-29 9:53 ` [PATCH 03/13] KVM: arm/arm64: add emulation model specific destroy function Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 8:51 ` Eric Auger
2015-06-09 8:51 ` Eric Auger
2015-06-28 19:14 ` Christoffer Dall
2015-06-28 19:14 ` Christoffer Dall
2015-05-29 9:53 ` [PATCH 04/13] KVM: arm64: Introduce new MMIO region for the ITS base address Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 8:52 ` Eric Auger
2015-06-09 8:52 ` Eric Auger
2015-06-11 15:12 ` Andre Przywara
2015-06-11 15:12 ` Andre Przywara
2015-05-29 9:53 ` [PATCH 05/13] KVM: arm64: handle ITS related GICv3 redistributor registers Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 8:52 ` Eric Auger
2015-06-09 8:52 ` Eric Auger
2015-06-12 17:03 ` Andre Przywara
2015-06-12 17:03 ` Andre Przywara
2015-05-29 9:53 ` [PATCH 06/13] KVM: arm64: introduce ITS emulation file with stub functions Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 9:23 ` Eric Auger
2015-06-09 9:23 ` Eric Auger
2015-05-29 9:53 ` [PATCH 07/13] KVM: arm64: implement basic ITS register handlers Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 13:34 ` Eric Auger
2015-06-09 13:34 ` Eric Auger
2015-06-28 19:36 ` Christoffer Dall
2015-06-28 19:36 ` Christoffer Dall
2015-05-29 9:53 ` [PATCH 08/13] KVM: arm64: add data structures to model ITS interrupt translation Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 15:59 ` Eric Auger
2015-06-09 15:59 ` Eric Auger
2015-05-29 9:53 ` [PATCH 09/13] KVM: arm64: handle pending bit for LPIs in ITS emulation Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-09 15:59 ` Eric Auger
2015-06-09 15:59 ` Eric Auger
2015-06-11 15:46 ` Andre Przywara
2015-06-11 15:46 ` Andre Przywara
2015-06-11 16:01 ` Marc Zyngier
2015-06-11 16:01 ` Marc Zyngier
2015-06-11 18:24 ` Eric Auger
2015-06-11 18:24 ` Eric Auger
2015-05-29 9:53 ` [PATCH 10/13] KVM: arm64: sync LPI properties and status between guest and KVM Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-11 17:44 ` Eric Auger
2015-06-11 17:44 ` Eric Auger
2015-06-28 19:33 ` Christoffer Dall
2015-06-28 19:33 ` Christoffer Dall
2015-05-29 9:53 ` [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-12 15:28 ` Eric Auger
2015-06-12 15:28 ` Eric Auger
2015-06-28 19:41 ` Christoffer Dall
2015-06-28 19:41 ` Christoffer Dall
2015-07-03 15:57 ` Andre Przywara
2015-07-03 15:57 ` Andre Przywara
2015-07-03 21:01 ` Christoffer Dall [this message]
2015-07-03 21:01 ` Christoffer Dall
2015-05-29 9:53 ` [PATCH 12/13] KVM: arm64: implement MSI injection in ITS emulation Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-11 17:43 ` Eric Auger
2015-06-11 17:43 ` Eric Auger
2015-07-06 16:46 ` Andre Przywara
2015-07-06 16:46 ` Andre Przywara
2015-07-07 8:13 ` Christoffer Dall
2015-07-07 8:13 ` Christoffer Dall
2015-05-29 9:53 ` [PATCH 13/13] KVM: arm64: enable ITS emulation as a virtual MSI controller Andre Przywara
2015-05-29 9:53 ` Andre Przywara
2015-06-12 16:05 ` Eric Auger
2015-06-12 16:05 ` Eric Auger
2015-06-18 8:43 ` Eric Auger
2015-06-18 8:43 ` Eric Auger
2015-06-18 14:22 ` Andre Przywara
2015-06-18 14:22 ` Andre Przywara
2015-06-18 15:03 ` Pavel Fedin
2015-06-18 15:03 ` Pavel Fedin
2015-06-18 19:20 ` Andre Przywara
2015-06-18 19:20 ` Andre Przywara
2015-06-08 6:53 ` [PATCH 00/13] arm64: KVM: GICv3 ITS emulation Pavel Fedin
2015-06-08 6:53 ` Pavel Fedin
2015-06-08 8:23 ` Marc Zyngier
2015-06-08 8:23 ` Marc Zyngier
2015-06-08 10:54 ` Pavel Fedin
2015-06-08 10:54 ` Pavel Fedin
2015-06-08 17:13 ` Marc Zyngier
2015-06-08 17:13 ` Marc Zyngier
2015-06-09 8:12 ` Eric Auger
2015-06-09 8:12 ` Eric Auger
2015-06-10 12:18 ` Pavel Fedin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150703210110.GA10878@cbox \
--to=christoffer.dall@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.