OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-dev] [RFC PATCH v2] virtio-spec: Introduce new Receive-Segment-Coalescing feature for guest

Add Yan in the loop.
Yan, there is some comments about the feature and header below, could you please have a look and share your idea? 

On 2016年06月14日 01:55, Michael S. Tsirkin wrote:

On Wed, Jun 08, 2016 at 08:43:28AM +0800, Wei Xu wrote:

Hi TCers,
Change in v2:
Feature 22 should not be used by device, replacing it with 41 and 42 for
VIRTIO_NET_GUEST_RSC4 and VIRTIO_NET_GUEST_RSC6.



Thanks!
Looks good, except for the comment below about hdr_len.
Could you comment on that please?

Thanks.


Do other TC members have any comments?



Index: content.tex
===================================================================
--- content.tex	(revision 569)
+++ content.tex	(working copy)
@@ -3086,6 +3086,11 @@

  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
      channel.
+
+\item[VIRTIO_NET_F_GUEST_RSC4 (41)] Driver can receive coalesced ipv4 tcp
packets.
+
+\item[VIRTIO_NET_F_GUEST_RSC6 (42)] Driver can receive coalesced ipv6 tcp
packets.
+
  \end{description}

  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network
Device / Feature bits / Feature bit requirements}
@@ -3247,8 +3252,11 @@
          u8 flags;
  #define VIRTIO_NET_HDR_GSO_NONE        0
  #define VIRTIO_NET_HDR_GSO_TCPV4       1
-#define VIRTIO_NET_HDR_GSO_UDP         3
-#define VIRTIO_NET_HDR_GSO_TCPV6       4
+#define VIRTIO_NET_HDR_GSO_UDP         2
+#define VIRTIO_NET_HDR_GSO_TCPV6       3


This is not a good idea I think.
We have code out there using the existing flags.

Also, pls look at 34a48579e4fb380604d06f0409db3851bd22d785 in linux -
some very old guests used
-#define VIRTIO_NET_HDR_GSO_TCPV4_ECN   2       // GSO frame, IPv4 TCP w/ ECN
so it's best not to touch it.

OK, thanks for point this out, i didn't notice that in the latest code.


Just start from 5, what's the issue with that?

Sure.




+#define VIRTIO_NET_HDR_RSC_NONE        4


What's wrong with using VIRTIO_NET_HDR_GSO_NONE?

It's ok to reuse that, but i have discussed this with Yan in a meeting,
we preferred a new flag for the feature to not make it overlapped with GSO, it is less confused. 


+#define VIRTIO_NET_HDR_RSC_TCPV4       5
+#define VIRTIO_NET_HDR_RSC_TCPV6       6
  #define VIRTIO_NET_HDR_GSO_ECN      0x80


Is ECN valid with RSC?
I think it's better not to touch ECN packets in RSC to keep these packets get passed asap, thus may reduce the latency, how do you think? 


          u8 gso_type;
          le16 hdr_len;
@@ -3256,6 +3264,9 @@
          le16 csum_start;
          le16 csum_offset;
          le16 num_buffers;
+        le16 coalesced_pkts;
+        le16 coalesced_size;
+        le16 dup_acks;


please specify that these are only present if RSC
is negotiated. And maybe name them as rsc_.

OK.


also specify what happens on transmit. Are different
headers used on xmit and receive?
I used to suppose using different headers, but maybe it is more concise to use the same one due to your comment below, 

Yan, how do you think about this?



  };
  \end{lstlisting}

@@ -3272,6 +3283,12 @@
  when VIRTIO_NET_F_MRG_RXBUF was negotiated; without that feature the
  structure was 2 bytes shorter.

+The legacy driver only presented \field{coalesced_pkts},
\field{coalesced_size}
+and \field{dup_ack} in the struct virtio_net_hdr when
VIRTIO_NET_F_GUEST_RSC4
+or VIRTIO_NET_F_GUEST_RSC6 was negotiated and a packet really has coalesced
+additional packet(s), without satisfying the 2 conditions the structure was
+6 bytes shorter.
+
  When using the legacy interface, the driver SHOULD ignore the
  \field{len} value in used ring entries for the transmit queues
  and the controlq queue.



I don't see why do you ever need to discuss legacy.
Legacy drivers never negotiate any RSC options.
Just drop this part?

Sure, my understanding is not correct about this.



@@ -3451,6 +3468,19 @@
  1514 bytes.  The 12-byte struct virtio_net_hdr is prepended to this,
  making for 65562 or 1526 bytes.

+If the VIRTIO_NET_F_GUEST_RSC4 feature is used, the maximum incoming
+packet will be up to 65550 bytes long (the maximum size of a TCP packet,
+plus the 14 byte ethernet header), otherwise 1514 bytes.
+The 16-byte struct virtio_net_hdr is prepended to this,
+making for 65566 or 1528 bytes.


Mostly same as GSO then? Why not add to that? Just add that virtio net is
longer then.
That's because the size for IPv4 and IPv6 will be different, but it's the same with GSO in the spec, so i added a separate comment here. 



BTW with vlans there are 4 extra bytes, and I think max tcp packet
is 65535 not 65536. Linux host will never produce bigger packets.
I think we should fix all that, but that is a separate issue.

Yes, you are right, will fix it.



+
+If the VIRTIO_NET_F_GUEST_RSC6 feature is used, the maximum incoming
+packet will be up to 65590 bytes long (the maximum size of a TCP packet,
+plus the 40-byte standard ipv6 header, plus the 14 byte ethernet header),
+otherwise 1514 bytes.
+The 16-byte struct virtio_net_hdr is prepended to this,
+making for 65606 or 1528 bytes.
+


This doesn't end with 40 bytes, does it? there are option headers too
...

So maybe not. Does RSC really need ability to pass to guest packets whose
length in bytes does not fit in 16 bit?
I forgot to comment RSC only makes sense to ip packets without any option, so all the packets with options will be bypassed. 



  \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types /
Network Device / Device Operation / Setting Up Receive Buffers}

  \begin{itemize}
@@ -3514,6 +3544,24 @@
    set: if so, device has validated the packet checksum.
    In case of multiple encapsulated protocols, one level of checksums
    has been validated.
+
+\item If one or both VIRTIO_NET_F_GUEST_RSC4, RSC6 were negotiated,
+  but it is a original packet which didn't coalesce any packet,
+  then the \field{flags} should be set to VIRTIO_NET_HDR_RSC_NONE.
+
+\item If the VIRTIO_NET_F_GUEST_RSC4, RSC6 options were negotiated,
+  and \field{flags} notifies it's a coalesced packet,


should be "and \field{flags} is either RSC4 or RSC6".

OK, thanks.



then
+  \field{coalesced_pkts} indicates how many packets are coalesced into
+  this packet.


what if \field{flags} is not RSC4 or RSC6?

I'm considering set it to zero by default.



+
+\item If the VIRTIO_NET_F_GUEST_RSC4, RSC6 options were negotiated,
+  and \field{flags} notifies it's a coalesced packet, then
+  \field{coalesced_size} indicates the real packet size after coalesced.

what if \field{flags} is not RSC4 or RSC6?

+
+\item If the VIRTIO_NET_F_GUEST_RSC4, RSC6 options were negotiated,
+  and \field{flags} notifies it's a coalesced packet, then
+  \field{dup_ack} indicates how many duplicated ack packets are
+  coalesced into this packet.

what if \field{flags} is not RSC4 or RSC6?

Same as above.

  \end{enumerate}

  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN
@@ -3626,6 +3674,15 @@
  If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor
  VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT
  rely on the packet checksum being correct.
+
+If one of VIRTIO_NET_F_GUEST_RSC4, RSC6 options have
+been negotiated, the driver MAY use \field{hdr_len} as a
+hint about the real virtio header size, for example, if a
+packets is an original packet, then the virtio header size
+should not contain the \field{coalesced_pkts}, \field{coalesced_size}
+and \field {dup_ack}, otherwise, these 3 fields should be included


OK so with RSC4/RSC6 the meaning of hdr_len changes?
Is this really important for performance?
If not I think it's nicer to keep it consistent:
	If one of the VIRTIO_NET_F_GUEST_TSO4, TSO6 or UFO options have been
	negotiated, the device
	SHOULD set hdr_len to a value not less than the length of the headers,
	including the transport header.
Great, i misunderstanding this part before, it's better to keep the consistency of this feature as the same as merge receive buffers. 

Yan, do you agree this?




in
+virito header size.


typo


Thanks.

+
  \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device /
Device Operation / Control Virtqueue}

  The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]