> From: Michael S. Tsirkin <m...@redhat.com>
> Sent: Tuesday, February 21, 2023 4:46 PM
>
> What is this information driver can't observe? It sees all the packets after
> all,
> we are not stripping tunneling headers.
Just the tunnel type.
If/when that tunnel header is stripped, it gets complicated where tunnel type
is still present in the virtio_net_hdr because hash_report_tunnel feature bit
is negotiated.
> I also don't really know what are upper layer drivers - for sure layering of
> drivers is not covered in the spec for now so I am not sure what do you mean
> by
> that. The risk I mentioned is leaking the information *on the network*.
>
Got it.
>
>
>
> > > > > \begin{lstlisting} struct virtio_net_rss_config {
> > > > > > > le32 hash_types;
> > > > > > > + le32 hash_tunnel_types;
> > > > > > This field is not needed as device config space advertisement
> > > > > > for the support
> > > > > is enough.
> > > > > >
> > > > > > If the intent is to enable hashing for the specific tunnel(s),
> > > > > > an individual
> > > > > command is better.
> > > > >
> > > > > new command? I am not sure why we want that. why not handle
> > > > > tunnels like we do other protocols?
> > > >
> > > > I didn't follow.
> > > > We probably discussed in another thread that to set M bits, it is
> > > > wise to avoid
> > > setting N other bits just to keep the command happy, where N >>> M
> > > and these N have a very strong relation in hw resource setup and packet
> steering.
> > > > Any examples of 'other protocols'?
> > >
> > > #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> > > #define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
> > > #define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
> > >
> > > this kind of thing.
> > >
> > > I don't see how a tunnel is different fundamentally. Why does it
> > > need its own field?
> >
> > Driver is in control to enable/disable tunnel based inner hash acceleration
> only when its needed.
> > This way certain data path hw parsers can be enabled/disabled.
> > Without this it will be always enabled even if there may not be any user of
> > it.
> > Device has scope to optimize this flow.
>
> I feel you misunderstand the question. Or maybe I misunderstand what you are
> proposing. So tunnels need their own bits. But why a separate field and not
> just
> more bits along the existing ones?
Because the hashing is not covering the outer header contents.
We may be still not discussing the same.
So let me refresh the context.
The question of discussion was,
Scenario:
1. device advertises the ability to hash on the inner packet header.
2. device prefers that driver enable it only when it needs to use this extra
packet parser in hardware.
There are 3 options.
a. Because the feature is negotiated, it means it is enabled for all the tunnel
types.
Pros:
1. No need to extend cvq cmd.
Cons:
1. device parser is always enabled, and the driver never uses it. This may
result in inferior rx performance.
b. Since the feature is useful in a narrow case of sw-based vxlan etc driver,
better not to enable hw for it.
Hence, have the knob to explicitly enable in hw.
So have the cvq command.
b.1 should it be combined with the existing command?
Cons:
a. when the driver wants to enable hash on inner, it needs to supply the exact
same RSS config as before. Sw overhead with no gain.
b. device needs to parse new command value, compare with old config, and drop
the RSS config, just enable inner hashing hw parser.
Or destroy the old rss config and re-apply. This results in weird behavior for
the short interval with no apparent gain.
b.2 should it be on its own command?
Pros:
a. device and driver doesn't need to bother about b.1.a and b.1.b.
b. still benefits from not always enabling hw parser, as this is not a common
case.
c. has the ability to enable when needed.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
