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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 08, 2023 at 04:18:23PM +0200, Eugenio Perez Martin wrote:
> 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?

Just allocate less, don't configure what you do not use.

> 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.

It's ok for all devices, and this is how it always worked.

> 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/
> >