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

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 07/23] KVM: arm64: vgic: Use atomics to count LPIs
Date: Wed, 14 Feb 2024 16:47:49 +0000	[thread overview]
Message-ID: <861q9f56x6.wl-maz@kernel.org> (raw)
In-Reply-To: <20240213093250.3960069-8-oliver.upton@linux.dev>

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.

-- 
Without deviation from the norm, progress is not possible.