On Wed, Feb 08, 2023 at 07:23:10PM +0800, Heng Qi wrote:
>
>
> 在 2023/2/8 下午6:04, Michael S. Tsirkin 写道:
> > On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> > >
> > > 在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
> > > > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > This patch supports configuring the parameters for a specified queue.
> > > > >
> > > > > When the traffic between queues is unbalanced, for example, one queue
> > > > > is busy and another queue is idle, then it will be very useful to
> > > > > control coalescing parameters at the queue granularity.
> > > > ethtool does not support this though, does it? what's the plan?
> > > Yes, ethtool does not support this function yet, and this function will
> > > not
> > > conflict with ethool.
> > > Our current goal is to let virtio-net support netdim first.
> > >
> > > > > Signed-off-by: Heng Qi <hen...@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanz...@linux.alibaba.com>
> > > > What I dislike about this interface is that if
> > > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
> > > > in the common case of same parameters for all queues
> > > > driver has to issue multiple commands.
> > > > I can see either a special vq index (0xffff ?) or a special
> > > > command used to set it for all queues.
> > > >
> > > >
> > > > > ---
> > > > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/content.tex b/content.tex
> > > > > index e863709..049c0e4 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > Types / Network Device / Feature bits
> > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> > > > > control
> > > > > channel.
> > > > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > > > > + notifications coalescing.
> > > > > +
> > > > > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> > > > > coalescing.
> > > > > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4
> > > > > packets.
> > > > > @@ -3140,6 +3143,7 @@ \subsubsection{Feature bit
> > > > > requirements}\label{sec:Device Types / Network Device
> > > > > \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > > \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> > > > > VIRTIO_NET_F_HOST_TSO6.
> > > > > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > and VIRTIO_NET_F_NOTF_COAL.
> > > > > \end{description}
> > > > > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > > > Types / Network Device / Feature bits / Legacy Interface: Feature
> > > > > bits}
> > > > > @@ -4488,16 +4492,21 @@ \subsubsection{Control
> > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > > > > send control commands for dynamically changing the coalescing
> > > > > parameters.
> > > > > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the
> > > > > driver
> > > > > +can send control commands to configure the coalescing parameters of a
> > > > > +specified receiveq or transmitq.
> > > > > \begin{lstlisting}
> > > > > struct virtio_net_ctrl_coal_rx {
> > > > > le32 rx_max_packets;
> > > > > le32 rx_usecs;
> > > > > + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL
> > > > > negotiated)
> > > > > };
> > > > > struct virtio_net_ctrl_coal_tx {
> > > > > le32 tx_max_packets;
> > > > > le32 tx_usecs;
> > > > > + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL
> > > > > negotiated)
> > > > > };
> > > > > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > > > I think it's a good idea to do this on top of Alvaro's patch
> > > > unifying these two structures.
> > > I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a
> > > good
> > > way for me to
> > > unify the two structures, since a patch should only do one thing.
> > >
> > > > > @@ -4507,17 +4516,34 @@ \subsubsection{Control
> > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > Coalescing parameters:
> > > > > \begin{itemize}
> > > > > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX
> > > > > notification.
> > > > > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX
> > > > > notification.
> > > > > +\item \field{rx_qid}: Index of which receiveq to change the
> > > > > coalescing parameters.
> > > > > + If the value is between 0 and 0x7FFF, it represents the index
> > > > > of the specified
> > > > > + receiveq. Otherwise, if the value is 0xFFFF, it indicates to
> > > > > change the
> > > > > + coalescing parameters for all receiveqs.
> > > > what if the index does not map to a receive queue?
> > > The spec mentioned
> > > "A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
> > > VIRTIO_NET_ERR if it was not able to change the parameters.",
> > > I think this belongs to this situation.
> > >
> > > Thanks.
> > Maybe but it's worth calling out. Because another interpretation
> > is that if qid matches rx queue we change rx parameters and
> > if it matches tx queue we change tx parameters.
> > There's a redundancy here I don't like different devices
> > might handle the error differently.
>
> Ok, I'll clarify here.
>
> > Maybe a new command is better?
>
> I agree, but there is one thing to confirm below.
>
> >
> > VIRTIO_NET_CTRL_NOTF_COAL_QUEUE_SET ?
> >
> > qid selects whether it's rx or tx.
>
> I think the qid you expressed here should refer to the virtqueue id?
> If qid represents the index of recieveq or transmitq, we can use
> VIRTIO_NET_CTRL_NOTF_COAL_RX_QUEUE_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_TX_QUEUE_SET.
>
> Which do you think is more suitable? I think the latter (qid means the id of
> receiveq or transmitq) seems to be more aligned with the current spec?
We do in fact have both. Calling vq number an index is well established
in the spec. index in the sense of "floor(vq index / 2)" is only used
once where we document the RSS indirection table, so only for receive.
I would be inclined to talk about vq numbers and change RSS to do that
too.
Saying somewhere in
\section{Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues}
that each virtqueue is associated with a 16 bit virtqueue index
can be a good idea, too.
--
MST
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
