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

[Yuri Benditovich <ybendito@redhat.com>]

---

From: "Jason Wang" <jasowang@redhat.com>
To: "Yuri Benditovich" <ybendito@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, "Yuri Benditovich" <yuri.benditovich@daynix.com>, virtio-comment@lists.oasis-open.org
Sent: Thursday, February 20, 2020 10:11:51 AM
Subject: Re: [virtio-comment] [PATCH v4 1/1] virtio-net: Define per-packet hash reporting feature

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

Also possible.

We'll need to define it as soon as we define in the spec the second feature affecting packet size.

There is also another point raised by Michael some time ago: different packet layouts for TX and RX.

Thanks

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/