Re: [virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash

[Heng Qi <hengqi@linux.alibaba.com>]

在 2023/2/22 上午7:18, Michael S. Tsirkin 写道:
> On Tue, Feb 21, 2023 at 10:32:11PM +0000, Parav Pandit wrote:
>>> From: Michael S. Tsirkin <mst@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.
> whoever strips off the tunnel has I imagine strip off the virtio net hdr
> too - everything else in it such as gso type refers to the outer packet.
>
>>> 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.
> I prefer b.1. With reporting of the tunnel type gone I don't see a
> fundamental difference between hashing over tunneling types and other
> protocol types we support.  It's just a flag telling device over which
> bits to calculate the hash. We don't have a separate command for hashing
> of TCPv6, why have it for vxlan?  Extending with more HASH_TYPE makes
> total sense to me, seems to fit better with the existing design and will
> make patch smaller.

+1.

It is infrequent to configure the *tunnel hash types* through commands, 
and when configuring the *hash types*,
the hash key and indirection table are not required too.

>
>