[v2,07/23] KVM: arm64: vgic: Use atomics to count LPIs

  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(-)
  

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.