Re: [PATCH RESEND 1/4] virtio-net: support transmit hash report

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

On Mon, Jun 24, 2024 at 01:09:04PM +0200, Steffen Trumtrar wrote:
> Virtio-net supports sharing the flow hash from device to driver on rx.
> Do the same in the other direction for robust routing and telemetry.
> 
> Experimental results mirror what the theory suggests: where IPv6
> FlowLabel is included in path selection (e.g., LAG/ECMP), flowlabel
> rotation on TCP timeout avoids the vast majority of TCP disconnects
> that would otherwise have occurred during link failures in long-haul
> backbones, when an alternative path is available.
> 
> Rotation can be applied to various bad connection signals, such as
> timeouts and spurious retransmissions. In aggregate, such flow level
> signals can help locate network issues.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  device-types/net/description.tex | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 61cce1f..eb2c08e 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -88,6 +88,8 @@ \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_TX_HASH(49)] Driver sends hash report
> +
>  \item[VIRTIO_NET_F_DEVICE_STATS(50)] Device can provide device-level statistics
>      to the driver through the control virtqueue.
>  
> @@ -641,6 +643,34 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>  
>  If VIRTIO_NET_HDR_F_NEEDS_CSUM is not set, the device MUST NOT
>  rely on the packet checksum being correct.
> +
> +\paragraph{Hash calculation for outgoing packets}
> +\label{sec:Device Types / Network Device / Device Operation / Packet Transmission / Hash calculation for outgoing packets }
> +
> +If the VIRTIO_NET_F_TX_HASH was negotiated and the packet includes a hash, the driver uses
> +the structure virtio_net_hdr_hash_ts instead of virtio_net_hdr and increases the \field{hdr_len} accordingly.

I don't get what does hdr_len have to do with it. It does not normally
include virtio net header and it does not look like your
patch changed it, either.


> +
> +\begin{lstlisting}
> +  struct virtio_net_hdr_hash_ts {
> +    struct {
> +      struct virtio_net_hdr hdr;

hdr is not part of hash, is it? why include it here?

> +      __le33 value;

le33?

> +      __le16 report;
> +      __le16 flow_state;

fields are left undocumented.

> +    } hash;
> +    __u32 reserved;

why do we need this? at least explain what it is and how is this
supposed to be handled, please.

> +  };
> +\end{lstlisting}
> +
> +The driver fills \field{hash_value} with the value of the calculated hash and \field{hash_report} with the report type of the calculated hash.

what are hash_value and hash_report?

> +
> +Possible values that the driver supports in \field{hash_report} are defined below.

considering how we have an extensive infrastructure for supported
hashes for rx, don't we want something like this for tx?

e.g. isn't there overhead associated with filling out this hash,
and doesn't driver benefit from knowing which ones can the
device use, exactly?

> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_REPORT_L4             (1 << 10)
> +#define VIRTIO_NET_HASH_REPORT_OTHER          (1 << 11)
> +\end{lstlisting}
> +
>  \paragraph{Packet Transmission Interrupt}\label{sec:Device Types / Network Device / Device Operation / Packet Transmission / Packet Transmission Interrupt}
>  
>  Often a driver will suppress transmission virtqueue interrupts
> 
> -- 
> 2.42.0
>