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

[Jason Wang <jasowang@redhat.com>]

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 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