From: Parav Pandit <parav@nvidia.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: "virtio-comment@lists.oasis-open.org"
<virtio-comment@lists.oasis-open.org>,
"cohuck@redhat.com" <cohuck@redhat.com>,
"hengqi@linux.alibaba.com" <hengqi@linux.alibaba.com>,
"xuanzhuo@linux.alibaba.com" <xuanzhuo@linux.alibaba.com>,
Shahaf Shuler <shahafs@nvidia.com>
Subject: [virtio-comment] RE: [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage
Date: Mon, 18 Sep 2023 13:12:37 +0000 [thread overview]
Message-ID: <PH0PR12MB548123B21F6D9F506DA11E52DCFBA@PH0PR12MB5481.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20230913044121-mutt-send-email-mst@kernel.org>
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, September 13, 2023 2:25 PM
>
I missed your email. Sorry for the delay.
Please find response below.
> On Sat, Sep 09, 2023 at 05:32:18PM +0300, Parav Pandit wrote:
> > Currently, a virtqueue must be enabled before driver has set the
> > DRIVER_OK status bit.
> >
> > spec citation to section "Driver Requirements: Device Initialization"
> >
> > "Perform device-specific setup, including discovery of virtqueues for
> > the device, optional per-bus setup, reading and possibly writing the
> > device’s virtio configuration space, and population of virtqueues."
> >
> > This implies that a virtqueue must be enabled before reaching the
> > DRIVER_OK stage. There was no explicit mention about ability to enable
> > the virtqueue after DRIVER_OK stage.
> >
> > In following usecases, creating a virtqueue after DRIVER_OK phase is
> > desired.
> >
> > 1. Dynamically create aq when administrative commands to be used.
> > 2. Dynamically create the net device tx/rxq when device is
> > opened when deploying for a container.
> > In a container, number of virtqueues to be used may be <= max queues.
> > 3. Dynamically create flow filter queues of netdevice when
> > ARFS or ethtool filters are enabled as listed in [1].
> > 4. Dynamically create rtc functionality related read virtqueue only
> > when net device when timestamping to be used.
> > 5. When XDP program is set, one can create additional XDP specific
> > queues without affecting existing queues.
> >
> > Hence, This patch introduces an existing queue enable and disable (aka
> > reset) facility and a new feature bit to explicitly indicate such
> > support by the device.
> >
> > With this feature, drivers can skip optional queues creation during
> > driver init time. For example, a Linux net device driver can
> > create/destroy the transmit and receive queues when net device's
> > ndo_open() and ndo_stop() callbacks are invoked respectively.
> >
> > As side effect, it also enables driver to not consume MSI-X vectors
> > for the PCI device at driver load time and utilize(enable) individual
> > vector when needed. This further helps on some cpus at high scale
> > where a interrupt line may not be available from the platform.
> >
> > [1]
> > https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.h
> > tml
>
>
> I'm not arguing against this functionality - Eugenio wanted it too - but the use-
> cases of MSI looks suspect.
>
> If you want to save on vectors isn't the thing to do actually to allow changing
> the vector in the PCI mapping? Or not even that - change vectors in the MSIX
> table.
>
MSIX vector in the device tells the about supported vectors.
In some systems, a system is running out of vectors even though device has it, and vq is not used either.
So it may be able to improve such cases, but I agree that it is not top priority. It is a byproduct gain.
More below.
>
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> > conformance.tex | 2 ++
> > content.tex | 54 ++++++++++++++++++++++++++++++++++++++++++++++--
> -
> > 2 files changed, 53 insertions(+), 3 deletions(-)
> >
> > diff --git a/conformance.tex b/conformance.tex index 863f9c5..07b0b7b
> > 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -77,6 +77,7 @@ \section{Conformance Targets}\label{sec:Conformance
> > / Conformance Targets} \item \ref{drivernormative:Basic Facilities of
> > a Virtio Device / Device Configuration Space} \item
> > \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
> > \item \ref{drivernormative:Basic Facilities of a Virtio Device /
> > Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > +Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > \item \ref{drivernormative:Basic Facilities of a Virtio Device /
> > Message Framing} \item \ref{drivernormative:Basic Facilities of a
> > Virtio Device / Virtqueues / The Virtqueue Descriptor Table} \item
> > \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues
> > / The Virtqueue Descriptor Table / Indirect Descriptors} @@ -164,6
> > +165,7 @@ \section{Conformance Targets}\label{sec:Conformance /
> > Conformance Targets} \item \ref{devicenormative:Basic Facilities of a
> > Virtio Device / Device Reset} \item \ref{devicenormative:Basic
> > Facilities of a Virtio Device / Device Configuration Space} \item
> > \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues
> > / Virtqueue Reset / Virtqueue Reset}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > +Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > \item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > Message Framing} \item \ref{devicenormative:Basic Facilities of a
> > Virtio Device / Virtqueues / The Virtqueue Descriptor Table} \item
> > \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues
> > / The Virtqueue Descriptor Table / Indirect Descriptors} diff --git
> > a/content.tex b/content.tex index 0a62dce..91aee25 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -102,7 +102,7 @@ \section{Feature Bits}\label{sec:Basic Facilities
> > of a Virtio Device / Feature B
> > \item[24 to 41] 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}
> > @@ -440,6 +440,42 @@ \subsubsection{Virtqueue
> > Re-enable}\label{sec:Basic Facilities of a Virtio Devic as during
> > initial virtqueue discovery, but optionally with different parameters.
> >
> > +\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is not negotiated, the virtqueues are
> > +enabled when the device is initialized, i.e. after the driver has
> > +ensured that the device has set the FEATURES_OK status bit and before
> > +the driver setting the DRIVER_OK status bit.
>
> Avoid passive voice pls. I think you mean driver enables not queues are
> enabled.
>
Yes, will fix it as,
the driver enables the virtqueues when device is initialized, ..
>
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid
> > +enabling the queue before setting the DRIVER_OK status bit; the
> > +driver can enable the queue after the driver has set the DRIVER_OK
> > +status bit. The queue enable mechanism is transport specific.
>
> I think you mean "specific queues" not "the queue".
>
Yes, will fix this too.
>
>
> Something else to consider is how this interacts with the ring reset feature. I
> think you must allow ring reset before driver ok too, right?
>
I couldn’t find a use case of why one wants to reset the queue which is not live until driver ok.
The _dynamic feature does not need to interact with ring reset, because _dynamic enables to do things after driver_ok.
>
> > +
> > +\devicenormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the device MUST allow
> > +enabling the virtqueue which was not enabled during the device
> > +initialization phase i.e. after the device has set the FEATURES_OK
> > +status bit and before the driver has set the DRIVER_OK status bit.
> > +
> > +\drivernormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, \begin{itemize} \item the
> > +driver MAY not enable all the virtqueues during the device
> > +initialization phase;
>
> MAY not sounds like it's not allowed. just drop this?
>
Since this is in the driver's normative, it is allowed by the device.
I think it is fine given the below listed item.
May be we swap both the items, that makes it clear?
>
> > +
> > +\item the driver MAY enable some or all the virtqueues after the driver has
> set the
> > + DRIVER_OK status bit.
> > +
> > +\item the driver MAY enable all the virtqueues during the device
> > +initialization phase; \end{itemize}
> > +
> > +The driver interested
>
> let's be more formal pls.
> - if it does not negotiate it SHOULD enable before
If it does not negotiate it MIST enable before.
> - if it does negotiate it MAY enable after
If it negotiates it MAY enable before or after.
Do you see a problem with adding "before?
>
> > in enabling and disabling the virtqueue dynamically
> > +after setting the DRIVER_OK status bit MUST negotiate
> > +VIRTIO_F_RING_DYNAMIC and VIRTIO_F_RING_RESET features.
>
> what does this have to do with reset?
>
This normative is probably not needed. Basically I wanted to capture a programming notion that,
If one wants to do enable/disable after driver ok, both feature bits are needed, only dynamic is not enough.
Should I move it out of normative and capture in the theory section?
>
> So what is your assumption on existing devices. Do you think no drivers enable
> before DRIVER_OK, or do you think some might?`
>
I probably didn’t follow the question.
My understanding is:
Existing drivers must enable before driver_ok.
And if they are enabled before driver_ok, only than one can reset them.
Existing devices may allow enable before or after driver_ok and driver has no way to know if the device really does that or not.
>
> > +
> > \input{split-ring.tex}
> >
> > \input{packed-ring.tex}
> > @@ -537,7 +573,9 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper
> >
> > \item\label{itm:General Initialization And Device Operation / Device
> Initialization / Device-specific Setup} Perform device-specific setup, including
> discovery of virtqueues for the
> > device, optional per-bus setup, reading and possibly writing the
> > - device's virtio configuration space, and population of virtqueues.
> > + device's virtio configuration space.
> > +
> > +\item\label{itm:General Initialization And Device Operation / Device
> Initialization / Virtqueue Setup} Configure and enable the virtqueues.
> >
> > \item\label{itm:General Initialization And Device Operation / Device
> Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit. At this point the
> device is
> > ``live''.
> > @@ -551,6 +589,10 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper The driver MUST NOT send any buffer
> > available notifications to the device before setting DRIVER_OK.
> >
> > +The driver MAY skip step
> > +\ref{itm:General Initialization And Device Operation / Device
> > +Initialization / Virtqueue Setup} when driver has negotiated
> VIRTIO_F_RING_DYNAMIC feature.
> > +
> > \subsection{Legacy Interface: Device
> > Initialization}\label{sec:General Initialization And Device Operation
> > / Device Initialization / Legacy Interface: Device Initialization}
> > Legacy devices did not support the FEATURES_OK status bit, and thus
> > did not have a graceful way for the device to indicate unsupported
> > feature @@ -569,7 +611,7 @@ \subsection{Legacy Interface: Device
> > Initialization}\label{sec:General Initializ Initialization / Re-read
> > FEATURES-OK} were omitted, and steps \ref{itm:General Initialization
> > And Device Operation / Device Initialization / Read feature bits},
> > -\ref{itm:General Initialization And Device Operation / Device
> > Initialization / Device-specific Setup} and \ref{itm:General
> > Initialization And Device Operation / Device Initialization / Set
> > DRIVER-OK}
> > +\ref{itm:General Initialization And Device Operation / Device
> > +Initialization / Device-specific Setup}, \ref{itm:General
> > +Initialization And Device Operation / Device Initialization /
> > +Virtqueue Setup} and \ref{itm:General Initialization And Device
> > +Operation / Device Initialization / Set DRIVER-OK}
> > were conflated.
> >
> > Therefore, when using the legacy interface:
> > @@ -872,8 +914,14 @@ \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_DYNAMIC(42)] This feature indicates that the
> > + driver can enable a virtqueue individually after the driver has
>
> virtqueues
Ack.
>
> > + set the status bit of DRIVER_OK.
> > + See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic
> Virtqueues}.
prev parent reply other threads:[~2023-09-18 13:12 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-09 14:32 [virtio-comment] [PATCH 0/2] Support enabling virtqueue after DRIVER_OK Parav Pandit
2023-09-09 14:32 ` [virtio-comment] [PATCH 1/2] conformance: Add missing virtqueue reset conformance references Parav Pandit
2023-09-13 7:36 ` [virtio-comment] " Xuan Zhuo
2023-09-09 14:32 ` [virtio-comment] [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage Parav Pandit
2023-09-13 7:40 ` [virtio-comment] " Xuan Zhuo
2023-09-13 12:20 ` Parav Pandit
2023-09-13 8:54 ` Michael S. Tsirkin
2023-09-18 13:12 ` Parav Pandit [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=PH0PR12MB548123B21F6D9F506DA11E52DCFBA@PH0PR12MB5481.namprd12.prod.outlook.com \
--to=parav@nvidia.com \
--cc=cohuck@redhat.com \
--cc=hengqi@linux.alibaba.com \
--cc=mst@redhat.com \
--cc=shahafs@nvidia.com \
--cc=virtio-comment@lists.oasis-open.org \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).