Re: [virtio-comment] [PATCH] virtio-net: Add support for correct hdr_len field.

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

On Sun, Feb 23, 2020 at 07:00:55AM +0000, Vitaly Mireyno wrote:
> 
> >-----Original Message-----
> >From: Michael S. Tsirkin <mst@redhat.com>
> >Sent: Thursday, 20 February, 2020 22:25
> >To: Vitaly Mireyno <vmireyno@marvell.com>
> >Cc: virtio-comment@lists.oasis-open.org; jasowang@redhat.com
> >Subject: Re: [virtio-comment] [PATCH] virtio-net: Add support for correct hdr_len field.
> >
> >----------------------------------------------------------------------
> >On Thu, Feb 20, 2020 at 10:18:32AM +0000, Vitaly Mireyno wrote:
> >>
> >> >-----Original Message-----
> >> >From: Michael S. Tsirkin <mst@redhat.com>
> >> >Sent: Thursday, 20 February, 2020 12:01
> >> >To: Vitaly Mireyno <vmireyno@marvell.com>
> >> >Cc: virtio-comment@lists.oasis-open.org; jasowang@redhat.com
> >> >Subject: [EXT] Re: [virtio-comment] [PATCH] virtio-net: Add support for correct hdr_len field.
> >> >
> >> >---------------------------------------------------------------------
> >> >- On Thu, Feb 20, 2020 at 09:51:17AM +0000, Vitaly Mireyno wrote:
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: virtio-comment@lists.oasis-open.org
> >> >> ><virtio-comment@lists.oasis-open.org> On Behalf Of Michael S.
> >> >> >Tsirkin
> >> >> >Sent: Thursday, 20 February, 2020 10:12
> >> >> >To: Vitaly Mireyno <vmireyno@marvell.com>
> >> >> >Cc: virtio-comment@lists.oasis-open.org; jasowang@redhat.com
> >> >> >Subject: Re: [virtio-comment] [PATCH] virtio-net: Add support for correct hdr_len field.
> >> >> >
> >> >> >------------------------------------------------------------------
> >> >> >---
> >> >> >- On Thu, Oct 24, 2019 at 03:24:43PM +0000, Vitaly Mireyno wrote:
> >> >> >> Some devices benefit from the knowledge of the exact header length for TSO processing.
> >> >> >> Add a feature bit for a driver that is capable of providing the
> >> >> >> correct header length for TSO
> >> >packets.
> >> >> >>
> >> >> >> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> >> >> >
> >> >> >So I looked at implementing this and maybe this was not such a good idea after all.
> >> >> >
> >> >> >How would we implement this in Linux?
> >> >> >Current code just puts skb_headlen there which is # of bytes in the linear buffer.
> >> >> >Which I guess it often the header, but not at all always.
> >> >> >
> >> >> >Should this have been limited to TSO packets?
> >> >> >
> >> >> >I also could not figure out how this is useful for the host.
> >> >> >Could you enlighten me pls?
> >> >>
> >> >> As see it, header length is essential for TSO, and maybe not so useful for non-TSO.
> >> >
> >> >So maybe we should limit this for when gso type is actually set?
> >>
> >> Do you mean the hdr_len field will be valid only for TSO packets, or it will be accurate only for TSO
> >packets?
> >> I'm fine with both options.
> >
> >Let's say accurate only for TSO.
> >
> >Can you write a spec patch like this?
> 
> Looking at the spec, I see that this is already the case - i.e. hdr_len is only required for TSO packets. Here:  "5.1.6.2 Packet Transmission  (3)"

And to quote that:

\end{note}

\item If the driver negotiated
  VIRTIO_NET_F_HOST_TSO4, TSO6 or UFO, and the packet requires
  TCP segmentation or UDP fragmentation, then \field{gso_type}
  is set to VIRTIO_NET_HDR_GSO_TCPV4, TCPV6 or UDP.
  (Otherwise, it is set to VIRTIO_NET_HDR_GSO_NONE). In this
  case, packets larger than 1514 bytes can be transmitted: the
  metadata indicates how to replicate the packet header to cut it
  into smaller packets. The other gso fields are set:

  \begin{itemize}
  \item If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated,
    \field{hdr_len} indicates the header length that needs to be replicated
    for each packet. It's the number of bytes from the beginning of the packet
    to the beginning of the transport payload.
    Otherwise, if the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated,
    \field{hdr_len} is a hint to the device as to how much of the header
    needs to be kept to copy into each packet, usually set to the
    length of the headers, including the transport header\footnote{Due to various bugs in implementations, this field is not useful
as a guarantee of the transport header size.
}.


Fine.


However in driver requirements it is not so clear:


If \field{gso_type} differs from VIRTIO_NET_HDR_GSO_NONE, then
the driver MUST also set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
\field{flags} and MUST set \field{gso_size} to indicate the
desired MSS.

If one of the VIRTIO_NET_F_HOST_TSO4, TSO6 or UFO options have
been negotiated:
\begin{itemize}
\item If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated,
        the driver MUST set \field{hdr_len} to a value equal to the length
        of the headers, including the transport header.

\item If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated,
        the driver SHOULD set \field{hdr_len} to a value
        not less than the length of the headers, including the transport
        header.
\end{itemize}

And so on. So it's a spec defect introduced by the new feature:
it used to be a weak SHOULD so I guess the assumption was,
it's ok to ask for it in any case even for !gso packets.

But now with a MUST it's a problem. I think the TC missed this
during review (I certainly did).
Can you try to address pls?



> 
> >> >
> >> >> To calculate the header length, I suppose a Linux driver could do something like:
> >> >> skb_transport_header(skb) + tcp_hdrlen(skb) - skb->data
> >
> >That's only good for TCP though.
> >
> >
> >> >
> >> >That's parsing the header in software. Why not have the card do it in hardware?
> >> >
> >>
> >> It depends on hw architecture. The architecture I'm familiar with, the hw can parse the header, but it
> >happens too late for the segmentation decision.
> >>
> >>
> >> >--
> >> >MST