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

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

Hi Parav,

On 2024-06-25 at 05:07 GMT, Parav Pandit <parav@nvidia.com> wrote:

> > From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Sent: Monday, June 24, 2024 4:39 PM
> > 
> > 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.
> >
> Can you please explain how does this related to the virtio-net device on transmit path?
> The value proposed here is only between the driver and device. It wont be part of the packet. Did I understand right?
> If so, which link do you refer to in above description? Virtio-net device link?
>

As I didn't develop that patch myself and I'm a little slow on the virtio-lingo I'm not sure
if *I* understand it right O:-)

So, in the kernel patch the TX hash report is set in the skb that is send with xmit_skb.
Does Qemu change that packet? Qemu is the "device", right?

>  
> > 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.
> > +
> > +\begin{lstlisting}
> > +  struct virtio_net_hdr_hash_ts {
> > +    struct {
> > +      struct virtio_net_hdr hdr;
> > +      __le33 value;
> > +      __le16 report;
> > +      __le16 flow_state;
> > +    } hash;
> > +    __u32 reserved;
> > +  };
> > +\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.
> > +
> It would be better to write it as \field{hash.value} because there is no hash_value field.
> 
> Instead of defining new structure, proposed feature bit can reuse the existing hash_value and hash_report fields.
> I guess that’s what you meant in above?
>

I mixed up the linux implementation with the spec.
Thanks, I updated the documentation completely according to your suggestions as I now
understand what is actually happening. 

> 
> > +Possible values that the driver supports in \field{hash_report} are defined
> > below.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_NET_HASH_REPORT_L4             (1 << 10)
> > +#define VIRTIO_NET_HASH_REPORT_OTHER          (1 << 11)
> > +\end{lstlisting}
> > +
> Please described _OTHER type of hash.
> 

Yes, will do.


Thanks,
Steffen

-- 
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |