| Message ID | 20240213093250.3960069-8-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Improvements to LPI injection | expand |
On Tue, 13 Feb 2024 09:32:44 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > Switch to using atomics for LPI accounting, allowing vgic_irq references > to be dropped in parallel. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/arm64/kvm/vgic/vgic-debug.c | 2 +- > arch/arm64/kvm/vgic/vgic-its.c | 4 ++-- > arch/arm64/kvm/vgic/vgic.c | 2 +- > include/kvm/arm_vgic.h | 4 ++-- > 4 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-debug.c b/arch/arm64/kvm/vgic/vgic-debug.c > index 85606a531dc3..389025ce7749 100644 > --- a/arch/arm64/kvm/vgic/vgic-debug.c > +++ b/arch/arm64/kvm/vgic/vgic-debug.c > @@ -149,7 +149,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist) > seq_printf(s, "vgic_model:\t%s\n", v3 ? "GICv3" : "GICv2"); > seq_printf(s, "nr_spis:\t%d\n", dist->nr_spis); > if (v3) > - seq_printf(s, "nr_lpis:\t%d\n", dist->lpi_list_count); > + seq_printf(s, "nr_lpis:\t%d\n", atomic_read(&dist->lpi_count)); > seq_printf(s, "enabled:\t%d\n", dist->enabled); > seq_printf(s, "\n"); > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index c68164d6cba0..048226812974 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -97,7 +97,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid, > goto out_unlock; > } > > - dist->lpi_list_count++; > + atomic_inc(&dist->lpi_count); > > out_unlock: > if (ret) > @@ -345,7 +345,7 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr) > * command). If coming from another path (such as enabling LPIs), > * we must be careful not to overrun the array. > */ > - irq_count = READ_ONCE(dist->lpi_list_count); > + irq_count = atomic_read(&dist->lpi_count); I'd like to propose an alternative approach here. I've always hated this "copy a bunch of INTIDs" thing, and the only purpose of this silly counter is to dimension the resulting array. Could we instead rely on an xarray marking a bunch of entries (the ones we want to 'copy'), and get the reader to clear these marks once done? Of course, we only have 3 marks, so that's a bit restrictive from a concurrency perspective, but since most callers hold a lock, it should be OK. What do you think? M.
Hey, On Wed, Feb 14, 2024 at 04:47:49PM +0000, Marc Zyngier wrote: > I'd like to propose an alternative approach here. I've always hated > this "copy a bunch of INTIDs" thing, Agree. > and the only purpose of this > silly counter is to dimension the resulting array. Well, we also use it to trivially print the number of LPIs for a particular vgic in the debug interface. > Could we instead rely on an xarray marking a bunch of entries (the > ones we want to 'copy'), and get the reader to clear these marks once > done? I think that'd work. I'm trying to convince myself we don't have bugs lurking in some of the existing usage of vgic_copy_lpi_list()... > Of course, we only have 3 marks, so that's a bit restrictive from a > concurrency perspective, but since most callers hold a lock, it should > be OK. They all hold *a* lock, but maybe not the same one! :) Maybe we should serialize the use of markers on the LPI list on the config_lock. A slight misuse, but we need a mutex since we're poking at guest memory. Then we can go through the whole N-dimensional locking puzzle and convince ourselves it is still correct.
On Wed, 14 Feb 2024 18:32:02 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hey, > > On Wed, Feb 14, 2024 at 04:47:49PM +0000, Marc Zyngier wrote: > > I'd like to propose an alternative approach here. I've always hated > > this "copy a bunch of INTIDs" thing, > > Agree. > > > and the only purpose of this > > silly counter is to dimension the resulting array. > > Well, we also use it to trivially print the number of LPIs for a > particular vgic in the debug interface. I think we can get survive this... ;-) > > > Could we instead rely on an xarray marking a bunch of entries (the > > ones we want to 'copy'), and get the reader to clear these marks once > > done? > > I think that'd work. I'm trying to convince myself we don't have bugs > lurking in some of the existing usage of vgic_copy_lpi_list()... > > > Of course, we only have 3 marks, so that's a bit restrictive from a > > concurrency perspective, but since most callers hold a lock, it should > > be OK. > > They all hold *a* lock, but maybe not the same one! :) Indeed. But as long as there isn't more than 3 locks (and that the xarray is OK being concurrently updated with marks), we're good! > Maybe we should serialize the use of markers on the LPI list on the > config_lock. A slight misuse, but we need a mutex since we're poking at > guest memory. Then we can go through the whole N-dimensional locking > puzzle and convince ourselves it is still correct. Maybe. This thing is already seeing so many abuses that one more may not matter much. Need to see how it fits in the whole hierarchy of GIC-related locks... Thanks, M.
On Wed, Feb 14, 2024 at 08:01:19PM +0000, Marc Zyngier wrote: > > > Of course, we only have 3 marks, so that's a bit restrictive from a > > > concurrency perspective, but since most callers hold a lock, it should > > > be OK. > > > > They all hold *a* lock, but maybe not the same one! :) > > Indeed. But as long as there isn't more than 3 locks (and that the > xarray is OK being concurrently updated with marks), we're good! Oh, you mean to give each existing caller their own mark? > > Maybe we should serialize the use of markers on the LPI list on the > > config_lock. A slight misuse, but we need a mutex since we're poking at > > guest memory. Then we can go through the whole N-dimensional locking > > puzzle and convince ourselves it is still correct. > > Maybe. This thing is already seeing so many abuses that one more may > not matter much. Need to see how it fits in the whole hierarchy of > GIC-related locks... It doesn't work. We have it that the config_lock needs to be taken outside the its_lock. Too many damn locks!
On Wed, 14 Feb 2024 23:01:04 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Wed, Feb 14, 2024 at 08:01:19PM +0000, Marc Zyngier wrote: > > > > Of course, we only have 3 marks, so that's a bit restrictive from a > > > > concurrency perspective, but since most callers hold a lock, it should > > > > be OK. > > > > > > They all hold *a* lock, but maybe not the same one! :) > > > > Indeed. But as long as there isn't more than 3 locks (and that the > > xarray is OK being concurrently updated with marks), we're good! > > Oh, you mean to give each existing caller their own mark? Well, each caller "class". Where "class" means "holding look 'foo'". Same lock, same mark. With a maximum of 3 (and I think we can get away with 2). > > > Maybe we should serialize the use of markers on the LPI list on the > > > config_lock. A slight misuse, but we need a mutex since we're poking at > > > guest memory. Then we can go through the whole N-dimensional locking > > > puzzle and convince ourselves it is still correct. > > > > Maybe. This thing is already seeing so many abuses that one more may > > not matter much. Need to see how it fits in the whole hierarchy of > > GIC-related locks... > > It doesn't work. We have it that the config_lock needs to be taken > outside the its_lock. > > Too many damn locks! Well, the joys of emulating highly complex HW with a braindead programming interface. I'd explore the above suggestion to avoid introducing a new lock, if at all possible. Thanks, M.
diff --git a/arch/arm64/kvm/vgic/vgic-debug.c b/arch/arm64/kvm/vgic/vgic-debug.c index 85606a531dc3..389025ce7749 100644 --- a/arch/arm64/kvm/vgic/vgic-debug.c +++ b/arch/arm64/kvm/vgic/vgic-debug.c @@ -149,7 +149,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist) seq_printf(s, "vgic_model:\t%s\n", v3 ? "GICv3" : "GICv2"); seq_printf(s, "nr_spis:\t%d\n", dist->nr_spis); if (v3) - seq_printf(s, "nr_lpis:\t%d\n", dist->lpi_list_count); + seq_printf(s, "nr_lpis:\t%d\n", atomic_read(&dist->lpi_count)); seq_printf(s, "enabled:\t%d\n", dist->enabled); seq_printf(s, "\n"); diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index c68164d6cba0..048226812974 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -97,7 +97,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid, goto out_unlock; } - dist->lpi_list_count++; + atomic_inc(&dist->lpi_count); out_unlock: if (ret) @@ -345,7 +345,7 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr) * command). If coming from another path (such as enabling LPIs), * we must be careful not to overrun the array. */ - irq_count = READ_ONCE(dist->lpi_list_count); + irq_count = atomic_read(&dist->lpi_count); intids = kmalloc_array(irq_count, sizeof(intids[0]), GFP_KERNEL_ACCOUNT); if (!intids) return -ENOMEM; diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c index e58ce68e325c..5988d162b765 100644 --- a/arch/arm64/kvm/vgic/vgic.c +++ b/arch/arm64/kvm/vgic/vgic.c @@ -122,7 +122,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) return; xa_erase(&dist->lpi_xa, irq->intid); - dist->lpi_list_count--; + atomic_dec(&dist->lpi_count); kfree(irq); } diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index aeff363e3ba6..71e9d719533b 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -273,10 +273,10 @@ struct vgic_dist { */ u64 propbaser; - /* Protects the lpi_list and the count value below. */ + /* Protects the lpi_list. */ raw_spinlock_t lpi_list_lock; struct xarray lpi_xa; - int lpi_list_count; + atomic_t lpi_count; /* LPI translation cache */ struct list_head lpi_translation_cache;
Switch to using atomics for LPI accounting, allowing vgic_irq references to be dropped in parallel. Signed-off-by: Oliver Upton <oliver.upton@linux.dev> --- arch/arm64/kvm/vgic/vgic-debug.c | 2 +- arch/arm64/kvm/vgic/vgic-its.c | 4 ++-- arch/arm64/kvm/vgic/vgic.c | 2 +- include/kvm/arm_vgic.h | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-)