Re: [virtio-comment] Re: [PATCH 1/2] virtio: introduce selective queue enabling

[Eugenio Perez Martin <eperezma@redhat.com>]

On Thu, Jun 8, 2023 at 2:24âPM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 6 Jun 2023 15:19:53 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, Jun 06, 2023 at 06:04:32PM +0000, Parav Pandit wrote:
> > > Hi Eugenio,
> > >
> > > > From: Eugenio Péez <eperezma@redhat.com>
> > > > Sent: Tuesday, June 6, 2023 1:55 PM
> > > >
> > > > This patch allows the driver to start the device (as set DRIVER_OK) with only
> > > > some queues enabled, and then enable another queues later.
> > > >
> > > > This is the current way to migrate net device state through control virtqueue, in
> > > > a software assisted framework with vDPA:
> > > > * First, only net CVQ is enabled at DRIVER_OK
> > > > * All the control commands (mac address, mq, etc) needed for the device to
> > > > behave the same as the source of migration are sent
> > > > * Finally all the dataplane queues are enabled.
> > > >
> > > > Signed-off-by: Eugenio Péez <eperezma@redhat.com>
> > > > ---
> > > >  content.tex | 15 +++++++++++++--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/content.tex b/content.tex
> > > > index d2ab9eb..ea4a6d8 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a
> > > > Virtio Device / Feature B  \begin{description}
> > > >  \item[0 to 23, and 50 to 127] Feature bits for the specific device type
> > > >
> > > > -\item[24 to 41] Feature bits reserved for extensions to the queue and
> > > > +\item[24 to 42] Feature bits reserved for extensions to the queue and
> > > >    feature negotiation mechanisms
> > > >
> > > > -\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
> > > > +\item[43 to 49, and 128 and above] Feature bits reserved for future
> > > > extensions.
> > > >  \end{description}
> > > >
> > > >  \begin{note}
> > > > @@ -398,6 +398,13 @@ \section{Virtqueues}\label{sec:Basic Facilities of a
> > > > Virtio Device / Virtqueues}  Every driver and device supports either the Packed
> > > > or the Split  Virtqueue format, or both.
> > > >
> > > > +\subsection{Virtqueue selective enabling}\label{sec:Basic Facilities of
> > > > +a Virtio Device / Virtqueues / Selective Virtqueue Enable}
> > > > +
> > > > +If the driver negotiates VIRTIO\_F\_RING\_ENABLE\_ANYTIME, it can
> > > > +enable individual virtqueues both before and after the DRIVER\_OK. The
> > > > +way to enable a virtqueue is transport specific, but it mimics the
> > > > +re-enabling after an hypothetical reset.
> > > > +
> > > >  \subsection{Virtqueue Reset}\label{sec:Basic Facilities of a Virtio Device /
> > > > Virtqueues / Virtqueue Reset}
> > > >
> > > >  When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
> > > > @@ -870,6 +877,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> > > > Feature Bits}
> > > >   \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
> > > >   handling features reserved for future use.
> > > >
> > > > +  \item[VIRTIO_F_RING_ENABLE_ANYTIME(42)] This feature indicates  that
> > > > + the driver can enable a queue both before and after DRIVER\_OK.
> > > > +  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Selective
> > > > Virtqueue Enable}.
> > > > +
> > >
> > > Can you please highlight, current spec normative which indicates that
> > > all queues must be enabled before DRIVER_OK and more queues cannot be
> > > enabled after DRIVER_OK?
> >
> >
> > This one:
> > \drivernormative{\subsection}{Device Initialization}{General Initialization And Device Operation / Device Initialization}
> >
> > it does not say "all queues" if you want to split hairs but I think
> > assuming it's safe to enable queues later is dangerous.
>
> Although there is no "all queues" here, if the queue is not ready, the device
> may use all the queues after "Device OK". So I also think it might be dangerous.
>
> But I also ran into this problem. We want the device to support 100 queues, but
> we also want to use only 16 queues by default. The user can then manually scale
> up to 100, if the user has a large memory.
>
> So I think, the point is that "num_queues" is the max and default value. Can we
> change "num_queues" to default, then we introduced a new value as max.
>
> Does this way solve your problem? @Eugenio
>

It would need a tweak, but it gives me an idea.

I think you're talking about what the device offers, isn't it? I would
need to modify it from the driver.

But it has an interesting question, how to allocate "less vrings" than
virtio_pci_common_cfg.num_queues? I guess it is ok to set all vq
addresses to a vring that never increments avail_idx? The driver still
needs to allocate memory for that one and lose start time configuring
them.

Maybe for net is valid to leave them unconfigured? it should only use
one vq pair and CVQ by default.

My proposal could be done at the virtio-net level, allowing
max_virtqueue_pairs to be 0 from the beginning. For example, allowing
the driver to write it before DRIVER_OK. In that case, no data vq is
enabled and only CVQ process requests after DRIVER_OK. After all the
config is written, the driver changes the number of active queues with
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET.

I think it is better solved in a generic way, but I'm ok with this
-net specific too.

Thanks!

> Thanks.
>
>
>
> >
> > E.g. vhost user backends tend to assume no new queues appear
> > after DRIVER_OK.
> >
> > --
> > MST
> >
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>