Re: [PATCH v1] s390/virtio_ccw: don't allocate/assign airqs for non-existing queues

[Halil Pasic]

On Fri, 4 Apr 2025 12:55:09 +0200
David Hildenbrand <david@xxxxxxxxxx> wrote:

> For virito-balloon, we should probably do the following:
> 
>  From 38e340c2bb53c2a7cc7c675f5dfdd44ecf7701d9 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@xxxxxxxxxx>
> Date: Fri, 4 Apr 2025 12:53:16 +0200
> Subject: [PATCH] virtio-balloon: Fix queue index assignment for
>   non-existing queues
> 
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
>   device-types/balloon/description.tex | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/device-types/balloon/description.tex b/device-types/balloon/description.tex
> index a1d9603..a7396ff 100644
> --- a/device-types/balloon/description.tex
> +++ b/device-types/balloon/description.tex
> @@ -16,6 +16,21 @@ \subsection{Device ID}\label{sec:Device Types / Memory Balloon Device / Device I
>     5
>   
>   \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtqueues}
> +
> +\begin{description}
> +\item[inflateq] Exists unconditionally.
> +\item[deflateq] Exists unconditionally.
> +\item[statsq] Only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
> +\item[free_page_vq] Only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
> +\item[reporting_vq] Only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.

s/is set/is negotiated/?

I think we should stick to "feature is offered" and "feature is
negotiated".

> +\end{description}
> +
> +\begin{note}
> +Virtqueue indexes are assigned sequentially for existing queues, starting
> +with index 0; consequently, if a virtqueue does not exist, it does not get
> +an index assigned. Assuming all virtqueues exist for a device, the indexes
> +are:
> +
>   \begin{description}
>   \item[0] inflateq
>   \item[1] deflateq
> @@ -23,12 +38,7 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
>   \item[3] free_page_vq
>   \item[4] reporting_vq
>   \end{description}
> -
> -  statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
> -
> -  free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
> -
> -  reporting_vq only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.
> +\end{note}
>   
>   \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
>   \begin{description}

Sounds good to me! But I'm still a little confused by the "holes". What
confuses me is that i can think of at least 2 distinct types of "holes":
1) Holes that can be filled later. The queue conceptually exists, but
   there is no need to back it with any resources for now because it is 
   dormant (it can be seen a hole in comparison to queues that need to
  materialize -- vring, notifiers, ...)
2) Holes that can not be filled without resetting the device: i.e. if
   certain features are not negotiated, then a queue X does not exist,
   but subsequent queues retain their index.

Can we have both kinds or was/will be 1) and/or 2) never a thing?

This patch would make sure that neither 1) nor 2) applies to
virtio-balloon, which is good. But we are talking about a
transport fix here, and I would like to eventually make sure
that the abstractions make sense.

That being said, I think we should proceed with this patch, because I
don't think Linux uses type 1) holes.

Regards,
Halil