Re: [virtio-comment] Re: [PATCH] virtio-net: Add equal-sized receive buffers feature

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

On Fri, Nov 01, 2019 at 03:22:26PM +0800, Jason Wang wrote:
> 
> On 2019/11/1 下午12:09, Michael S. Tsirkin wrote:
> > On Thu, Oct 31, 2019 at 10:46:55AM +0000, Vitaly Mireyno wrote:
> > > The feature is limited to receive buffers only.
> > > A driver decides on receive buffer length. The only limitation is that this length has to be the same for all receive virtqueue buffers.
> > > The driver configures receive buffer length to the device during device initialization, and the device reads it and may use it for optimal operation.
> > > No changes for transmit buffers.
> > > 
> > > -----Original Message-----
> > > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-open.org> On Behalf Of Jason Wang
> > > Sent: Thursday, 31 October, 2019 12:15
> > > To: Vitaly Mireyno <vmireyno@marvell.com>; virtio-comment@lists.oasis-open.org
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Subject: [virtio-comment] Re: [PATCH] virtio-net: Add equal-sized receive buffers feature
> > > 
> > > 
> > > On 2019/10/31 下午5:23, Vitaly Mireyno wrote:
> > > > Some devices benefit from working with receive buffers of a predefined constant length.
> > > > Add a feature for drivers that allocate equal-sized receive buffers, and devices that benefit from predefined receive buffers length.
> > > > 
> > > > Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> > > > ---
> > > >    content.tex | 29 +++++++++++++++++++++++++++--
> > > >    1 file changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > Is there any other networking device that has this feature?
> > > 
> > > 
> > > > diff --git a/content.tex b/content.tex index b1ea9b9..c9e67c8 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -2811,6 +2811,10 @@ \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_CONST_RXBUF_LEN(58)] Driver allocates all receive buffers
> > > > +    of the same constant length. Device benefits from working with
> > > > +    receive buffers of equal length.
> > > > +
> > Problem is, I don't think linux will use this for skbs since it breaks
> > buffer accounting. This is because it is important to make skbs
> > as small as you can. So even if you see "device would benefit"
> > there is no way to balance this with the benefit to linux.
> > How do you know which benefit is bigger?
> 
> 
> I guess the idea is e.g for Linux driver, it can refuse this feature.

Okay. What uses it?

> 
> > 
> > You also never explained how does device benefit. My guess is this
> > allows device to calculate the # of descriptors to fetch
> > that are needed for a packet. Right?
> > Assuming this, I think a rough estimate should be enough.
> > If device fetches too much it can discard extra, if it does not
> > fetch enough it can fetch extra.
> > 
> > Let us not forget that express packets are 256 bytes so
> > up to 16 descriptors fit in a packet, there is no
> > benefit most of the time in knowing whether e.g. 1 or 2
> > descriptors are needed.
> > 
> > 
> > Let us not forget these are buffers, not descriptors.
> 
> 
> I guess maybe the initial motivation is constant descriptor length.


That conflicts with requirement framing is up to driver.


> 
> > 
> > So device does not really know the exact # of descriptors:
> > current drivers do 1 descriptor per buffer but nothing
> > prevents more.
> > 
> > 
> > Thoughts?
> > 
> > 
> > > >    \item[VIRTIO_NET_F_RSC_EXT(61)] Device can process duplicated ACKs
> > > >        and report number of coalesced segments and duplicated ACKs
> > > > @@ -2854,8 +2858,8 @@ \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network
> > > >    \subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
> > > >    \label{sec:Device Types / Block Device / Feature bits / Device
> > > > configuration layout}
> > > > -Three driver-read-only configuration fields are currently defined.
> > > > The \field{mac} address field -always exists (though is only valid if
> > > > VIRTIO_NET_F_MAC is set), and
> > > > +The driver-read-only \field{mac} address field always exists (though
> > > > +is only valid if VIRTIO_NET_F_MAC is set), and
> > > >    \field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
> > > >    read-only bits (for the driver) are currently defined for the status field:
> > > >    VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > > > @@ -2875,12 +2879,17 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
> > > >    VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
> > > >    use.
> > > > +The device-read-only field \field{rx_buf_len} only exists if
> > > 
> > > Should be driver-read-only.
> > > 
> > > 
> > > > +VIRTIO_NET_F_CONST_RXBUF_LEN is negotiated. This field specifies the
> > > > +receive buffers length.
> > > > +
> > > >    \begin{lstlisting}
> > > >    struct virtio_net_config {
> > > >            u8 mac[6];
> > > >            le16 status;
> > > >            le16 max_virtqueue_pairs;
> > > >            le16 mtu;
> > > > +        le32 rx_buf_len;
> > > >    };
> > > >    \end{lstlisting}
> > > > @@ -2933,6 +2942,13 @@ \subsection{Device configuration
> > > > layout}\label{sec:Device Types / Network Device
> > > >    A driver SHOULD negotiate the VIRTIO_NET_F_STANDBY feature if the device offers it.
> > > > +A driver SHOULD accept the VIRTIO_NET_F_CONST_RXBUF_LEN feature if offered.
> > > > +
> > > > +If VIRTIO_NET_F_CONST_RXBUF_LEN feature has been negotiated, the
> > > > +driver MUST set \field{rx_buf_len}.
> > > 
> > > I think it's device that set the field?
> > Makes more sense for the driver, but if you want this set
> > e.g. before buffers are added, you must say so.
> > 
> > > > +
> > > > +A driver MUST NOT modify \field{rx_buf_len} once it has been set.
> > > > +
> > 
> > This seems very unflexible. I can see how e.g. XDP would benefit from
> > big buffers while skbs benefit from small buffers.
> > 
> > This calls for ability to change this.
> 
> 
> Yes, but it requires non trivial cleanups for the old length and place them
> with new ones.
> 
> Thanks


Let's see:
1	- making buffer smaller: just update config space,
	  then make new buffers smaller

2	- making buffers bigger: add larger buffers,
	once all small ones are consumed update config space

2 is tricky I agree. Thoughts?



> 
> > 
> > > >    \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
> > > >    \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
> > > >    When using the legacy interface, transitional devices and drivers @@
> > > > -3241,6 +3257,11 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
> > > >    If VIRTIO_NET_F_MQ is negotiated, each of receiveq1\ldots receiveqN
> > > >    that will be used SHOULD be populated with receive buffers.
> > > > +If VIRTIO_NET_F_CONST_RXBUF_LEN feature has been negotiated, the
> > > > +driver MUST initialize all receive virtqueue descriptors \field{len}
> > > > +field with the value configured in \field{rx_buf_len} device
> > > > +configuration field, and allocate receive buffers accordingly.
> > > > +
> > > >    \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device
> > > > Types / Network Device / Device Operation / Setting Up Receive
> > > > Buffers}
> > > >    The device MUST set \field{num_buffers} to the number of descriptors
> > > > used to @@ -3396,6 +3417,10 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > > >    checksum (in case of multiple encapsulated protocols, one level
> > > >    of checksums is validated).
> > > > +If VIRTIO_NET_F_CONST_RXBUF_LEN has been negotiated, the device MAY
> > > > +use \field{rx_buf_len} as a buffer length (instead of reading it from
> > > > +virtqueue descriptor \field{len} field).
> > > 
> > > Is this safe? What if driver submit a small buffer, then device can read more than what is allowed?
> > > 
> > > Thanks
> > > 
> > > 
> > > > +
> > > >    \drivernormative{\paragraph}{Processing of Incoming
> > > >    Packets}{Device Types / Network Device / Device Operation /
> > > >    Processing of Incoming Packets}
> > > > --
> > > 
> > > This publicly archived list offers a means to provide input to theOASIS Virtual I/O Device (VIRTIO) TC.In order to verify user consent to the Feedback License terms andto minimize spam in the list archive, subscription is requiredbefore posting.Subscribe: virtio-comment-subscribe@lists.oasis-open.orgUnsubscribe: virtio-comment-unsubscribe@lists.oasis-open.orgList help: virtio-comment-help@lists.oasis-open.orgList archive: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.oasis-2Dopen.org_archives_virtio-2Dcomment_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=KkXMgr3LRg5F7_7jB6uHCHA2n_Tn6303zez2N4VkSfU&s=ZvYuXs3LuhkeJxW_zoADv1L4RisDyGH_vp7scO91w9s&e= Feedback License: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.org_who_ipr_feedback-5Flicense.pdf&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=KkXMgr3LRg5F7_7jB6uHCHA2n_Tn6303zez2N4VkSfU&s=7Hf-O4ss-rLdNJsAbIOg5ruheBcfAYFdXWuks_HJfFQ&e= List Guidelines: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.org_policies-2Dguidelines_mailing-2Dlists&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=KkXMgr3LRg5F7_7jB6uHCHA2n_Tn6303zez2N4VkSfU&s=7oMwlBy4fdYwjT2_f9BTwHRm2GFtIYL-x8e4C0jlrpM&e= Committee: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.org_committees_virtio_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=KkXMgr3LRg5F7_7jB6uHCHA2n_Tn6303zez2N4VkSfU&s=-K2akZ06PY7r4TquQY4T4UWdZ6JyAsBDFZIEZFNsWHk&e= Join OASIS: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-2Dopen.org_join_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7xgI&m=KkXMgr3LRg5F7_7jB6uHCHA2n_Tn6303zez2N4VkSfU&s=lR1SlzXT71tjw7DDuD6XaHefJQSTBCAvRcdp45ureB8&e=
> > > 
> > 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/
> > 

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/