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

* RE: [virtio-comment] Re: [PATCH] virtio-net: Add equal-sized receive buffers feature
@ 2019-10-31 10:46 Vitaly Mireyno
  2019-11-01  4:09 ` Michael S. Tsirkin
  0 siblings, 1 reply; 28+ messages in thread
From: Vitaly Mireyno @ 2019-10-31 10:46 UTC (permalink / raw)
  To: Jason Wang, virtio-comment; +Cc: Michael S. Tsirkin

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.
> +
>   \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?

> +
> +A driver MUST NOT modify \field{rx_buf_len} once it has been set.
> +
>   \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= 

^ permalink raw reply	[flat|nested] 28+ messages in thread