On Tue, Nov 29, 2022 at 1:34 PM Michael S. Tsirkin <[email protected]> wrote: > > On Tue, Nov 29, 2022 at 11:47:19AM +0800, Jason Wang wrote: > > On Mon, Nov 28, 2022 at 1:33 PM Heng Qi <[email protected]> wrote: > > > > > > On Mon, Nov 28, 2022 at 11:52:23AM +0800, Jason Wang wrote: > > > > > > > > 在 2022/11/28 11:14, Heng Qi 写道: > > > > >On Fri, Nov 25, 2022 at 12:16:05PM +0800, Jason Wang wrote: > > > > >>On Tue, Nov 22, 2022 at 5:08 PM Heng Qi <[email protected]> > > > > >>wrote: > > > > >>>When VIRTIO_NET_F_RSS is negotiated and the tunnel is used to > > > > >>>encapsulate the packets, the hash calculated using the outer header > > > > >>>of the receive packets is always fixed for the same flow packets, > > > > >>>i.e. they will be steered to the same receive queue. > > > > >>> > > > > >>>We add a VIRTIO_NET_HASH_TYPE_GRE_INNER bitmask in > > > > >>>\field{hash_types}, > > > > >>>which instructs the device to calculate the hash using the inner > > > > >>>headers of GRE-encapsulated packets, and a VIRTIO_NET_HASH_REPORT_GRE > > > > >>>value in \field{hash_tunnel} to report packet type when calculating > > > > >>>hash over the inner header. > > > > >>So I think we need a new feature bit for this to keep migration > > > > >>compatibility. > > > > >> > > > > >If we consider adding feature negotiation for this, it will be > > > > >explained > > > > >more below. > > > > > > > > > >>>Signed-off-by: Heng Qi <[email protected]> > > > > >>>Signed-off-by: Xuan Zhuo <[email protected]> > > > > >>>--- > > > > >>>v1: > > > > >>> 1. Remove the patch for the bitmask fix. @Michael S. Tsirkin > > > > >>> 2. Clarify some paragraphs. @Jason Wang > > > > >>> 3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. > > > > >>> @Yuri Benditovich > > > > >>> > > > > >>> content.tex | 140 > > > > >>> ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > > > >>> 1 file changed, 135 insertions(+), 5 deletions(-) > > > > >>> > > > > >>>diff --git a/content.tex b/content.tex > > > > >>>index e863709..fba0c7d 100644 > > > > >>>--- a/content.tex > > > > >>>+++ b/content.tex > > > > >>>@@ -3095,7 +3095,7 @@ \subsection{Feature bits}\label{sec:Device > > > > >>>Types / Network Device / Feature bits > > > > >>> to several segments when each of these smaller packets has UDP > > > > >>> header. > > > > >>> > > > > >>> \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet > > > > >>> hash > > > > >>>- value and a type of calculated hash. > > > > >>>+ value, a type of calculated hash and a tunnel packet type. > > > > >>> > > > > >>> \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact > > > > >>> \field{hdr_len} > > > > >>> value. Device benefits from knowing the exact header length. > > > > >>>@@ -3386,7 +3386,8 @@ \subsection{Device Operation}\label{sec:Device > > > > >>>Types / Network Device / Device O > > > > >>> le16 num_buffers; > > > > >>> le32 hash_value; (Only if VIRTIO_NET_F_HASH_REPORT > > > > >>> negotiated) > > > > >>> le16 hash_report; (Only if VIRTIO_NET_F_HASH_REPORT > > > > >>> negotiated) > > > > >>>- le16 padding_reserved; (Only if VIRTIO_NET_F_HASH_REPORT > > > > >>>negotiated) > > > > >>>+ le8 hash_tunnel; (Only if VIRTIO_NET_F_HASH_REPORT > > > > >>>negotiated) > > > > >>It's better not limit this to be tunnel only unless we limit the same > > > > >>for hash_config. > > > > >Maybe we can use the \field{hash_report_ex} instead of > > > > >\field{hash_tunnel}？ > > > > > > > > > > > > Probably. > > > > > > > > > > > > > > > > > >>Btw, this needs an independent fix. I wonder if we need a dedicated > > > > >>feature bit VIRTIO_NET_F_HASH_REPORT_EX and documenting that device > > > > >>SHOULD offer HASH_REPORT_EX along with HASH_REPORT. Then we can do GRE > > > > >>tunnel hash report on top? (Or doing GRE first and fix the mismatch on > > > > >>top) > > > > >> > > > > >For this, we have the following ideas: > > > > > > > > > >1. Considering our actual business application scenarios, the current > > > > >mainstream > > > > > tunnel-encapsulated technologies are mainly GRE and VXLAN, so we > > > > > are also > > > > > working on VXLAN. > > > > > > > > > >2. To keep migration compatibility, we can add a > > > > >VIRTIO_NET_F_HASH_GRE_INNER > > > > > feature bit (it depends on VIRTIO_NET_F_RSS). If it is negotiated, > > > > > this > > > > > means that the device calculates the hash based on the inner > > > > > header of the > > > > > GRE-encapsulated packet. We assume that the inner header in GRE is > > > > > TCPv4, > > > > > at this time \field{hash_types} needs to include > > > > > (VIRTIO_NET_HASH_TYPE_GRE_INNER | VIRTIO_NET_HASH_TYPE_TCPv4). > > > > > Besides, > > > > > if VIRTIO_NET_F_HASH_REPORT is also negotiated, then > > > > > \field{hash_report} > > > > > should be set to VIRTIO_NET_HASH_REPORT_TCPv4, and field > > > > > \field{hash_report_ex} > > > > > should be set to VIRTIO_NET_HASH_REPORT_GRE. > > > > > > > > > > > > One question here, if I was not wrong, hash_report is sufficient for > > > > GRE and VXLAN now. So that's why I think they should be indenepent > > > > patch. > > > > > > > > > > As discussed in > > > https://lists.oasis-open.org/archives/virtio-dev/202211/msg00064.html, > > > \field{hash_report} is an integer rather than a bitmask. > > > > Ok, I see. > > > > > On the premise that > > > VIRTIO_NET_F_HASH_GRE_INNER is negotiated, assuming that the inner header > > > of the GRE packet > > > is TCPv4 and we only have \field{hash_report} instead of > > > \field{hash_report_ex}, then we > > > need to set VIRTIO_NET_HASH_REPORT_GRE(10) in \field{hash_report} along > > > with > > > VIRTIO_NET_HASH_REPORT_TCPv4(2). At this point \field{hash_report} should > > > be (2+10=12). > > > > > > However, if the inner header of another VXLAN packet is IPv4, and > > > VIRTIO_NET_HASH_REPORT_VXLAN > > > is 11 (following VIRTIO_NET_HASH_REPORT_GRE(10), like below), then > > > \field{hash_report} is > > > (1+11=12). Then how does the driver distinguish that 12 belongs to the > > > above which situation? > > > > > > Suppose the report type is as follows: > > > \begin{lstlisting} > > > #define VIRTIO_NET_HASH_REPORT_NONE 0 > > > #define VIRTIO_NET_HASH_REPORT_IPv4 1 > > > #define VIRTIO_NET_HASH_REPORT_TCPv4 2 > > > #define VIRTIO_NET_HASH_REPORT_UDPv4 3 > > > #define VIRTIO_NET_HASH_REPORT_IPv6 4 > > > #define VIRTIO_NET_HASH_REPORT_TCPv6 5 > > > #define VIRTIO_NET_HASH_REPORT_UDPv6 6 > > > #define VIRTIO_NET_HASH_REPORT_IPv6_EX 7 > > > #define VIRTIO_NET_HASH_REPORT_TCPv6_EX 8 > > > #define VIRTIO_NET_HASH_REPORT_UDPv6_EX 9 > > > #define VIRTIO_NET_HASH_REPORT_GRE 10 > > > #define VIRTIO_NET_HASH_REPORT_VXLAN 11 > > > \end{lstlisting} > > > > > > So it seems more reasonable to include tunnel-related report types in > > > \field{hash_report_ex}, > > > > Ok, I think I got this, if we go this way, hash_report_tunnel might be > > better. > > I agree. > > > In the long run, the mismatching behaviour of hash_config and > > hash_report might end up more burden in the maintenance. I wonder if > > it's worth it to make hash_report a bitmask that matches hash_config. > > That seems to ease everything a lot. > > > > Thanks > > Maybe but I don't like making this work being blocked by this new idea - > that's reworking this feature quite a lot.
Ok, then that's fine. > Do you have the time to work on this idea short term? Probably not. Thanks > > -- > MST > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
