Re: [virtio-comment] [PATCH v4 1/1] virtio-net: Define per-packet hash reporting feature

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

On Thu, Feb 20, 2020 at 04:11:51PM +0800, Jason Wang wrote:
> 
> On 2020/2/20 äå3:41, Yuri Benditovich wrote:
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> >     *From: *"Jason Wang" <jasowang@redhat.com>
> >     *To: *"Michael S. Tsirkin" <mst@redhat.com>
> >     *Cc: *"Yuri Benditovich" <yuri.benditovich@daynix.com>,
> >     virtio-comment@lists.oasis-open.org
> >     *Sent: *Thursday, February 20, 2020 8:38:50 AM
> >     *Subject: *Re: [virtio-comment] [PATCH v4 1/1] virtio-net: Define
> >     per-packet hash reporting feature
> > 
> > 
> >     On 2020/2/19 äå10:23, Michael S. Tsirkin wrote:
> >     > On Wed, Feb 19, 2020 at 08:27:10PM +0800, Jason Wang wrote:
> >     >> On 2020/2/19 äå3:53, Yuri Benditovich wrote:
> >     >>> Â ÂThe device MUST set
> >     \field{rss_max_indirection_table_length} to at least 128, if it offers
> >     >>> Â ÂVIRTIO_NET_F_RSS.
> >     >>> @@ -3195,6 +3180,8 @@ \subsection{Device
> >     Operation}\label{sec:Device Types / Network Device / Device O
> >     >>> Â Â Â Â Â Âle16 csum_start;
> >     >>> Â Â Â Â Â Âle16 csum_offset;
> >     >>> Â Â Â Â Â Âle16 num_buffers;
> >     >>> + Â Â Â Âle32 hash_value; (Only if VIRTIO_NET_F_HASH_REPORT
> >     negotiated)
> >     >>> + Â Â Â Âle16 hash_type; Â(Only if VIRTIO_NET_F_HASH_REPORT
> >     negotiated)
> >     >>> Â Â};
> >     >>> Â Â\end{lstlisting}
> >     >> A question here:
> >     >>
> >     >> Consider we introduce VIRTIO_NET_F_FEATURE_INFORMATION in the
> >     future:
> >     >>
> >     >> le32 hash_type; // VIRTIO_NET_F_HASH_REPORT
> >     >> le32 feature_information; // VIRTIO_NET_F_FEATURE_INFORMATION
> >     >>
> >     >> What happens if HASH_REPORT is not negotiated, I believe we
> >     expect a stable
> >     >> ABI(offset) here for feature_information?
> >     >>
> >     >> Thanks
> >     >>
> >     > We'll have to decide at that point ... any better ideas?
> > 
> > 
> >     Not sure but something that is self descriptive? (which could be an
> >     overkill for fields that only need few bytes).
> > 
> > The problem is that the driver typically wants to know the header size
> > from the beginning to configure SG table.
> 
> 
> Yes, so still the above example. Consider a driver only support
> VIRTIO_NET_F_FEATURE_INFORMATION, how to determine the size of the vnet
> header?
> 
> Stable offset is simpler but may waste space.

So we have:
                dev->needed_headroom = vi->hdr_len;

simply put we are going to waste space on the max header size anyway.
Dynamically sizing the header for each packet is pointless,
won't save memory.

> 
> > So any self-descriptive layout seems good but in practice not so usable,
> > IMO.
> 
> 
> I think then it can calculate the header length based on the feature
> negotiated.
> 
> hdr_len = sizeof(vnet_hdr) + VIRTIO_NET_F_HASH_REPORT ? 4 : 0 +
> VIRTIO_NET_F_FEAUTER_INFORMATION ? 4 : 0
> 
> Thanks