[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
å 2023/6/30 äå9:36, Parav Pandit åé:
From: Heng Qi <hengqi@linux.alibaba.com> Sent: Thursday, June 29, 2023 8:54 PM On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:From: Michael S. Tsirkin <mst@redhat.com> Sent: Thursday, June 29, 2023 7:48 AMstruct virtio_net_hash_config reserved is fine.+1. Inner header hash is orthogonal to RSS, and it's fine to have its own structure and commands. There is no need to send additional RSS fields when we configure inner header hash. Thanks.Not RSS, hash calculations. It's not critical, but I note that practically you said you will enable this with symmetric hash so it makes sense to me to send this in the same command with the key.In the v19, we have, +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQalong with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.So it is done along with rss, so in same struct as rss config is fine.Do you mean having both virtio_net_rss_config and virtio_net_hash_config have enabled_hash_types? Like this: struct virtio_net_rss_config { le32 hash_types; le16 indirection_table_mask; struct rss_rq_id unclassified_queue; struct rss_rq_id indirection_table[indirection_table_length]; le16 max_tx_vq; u8 hash_key_length; u8 hash_key_data[hash_key_length]; + le32 enabled_tunnel_types; }; struct virtio_net_hash_config { le32 hash_types; - le16 reserved[4]; + le32 enabled_tunnel_types; + le16 reserved[2]; u8 hash_key_length; u8 hash_key_data[hash_key_length]; }; If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types in virtio_net_rss_config will follow the variable length field and cause misalignment. If we let the inner header hash reuse the virtio_net_hash_config structure, it can work, but the only disadvantage is that the configuration of the inner header hash and *RSS*(not hash calculations) becomes somewhat coupled. Just imagine: If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1. then if we only want to configure the inner header hash (such as enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone; 2. but then if we want to configure the inner header hash and RSS (such as indirection table), we need to send all virtio_net_rss_config and virtio_net_hash_config once, because virtio_net_rss_config now does not carry enabled_tunnel_types due to misalignment. So, I think the following structure will make it clearer to configure inner header hash and RSS/hash calculation. But in any case, if we still propose to reuse virtio_net_hash_config proposal, I am ok, no objection: 1. The supported_tunnel_types are placed in the device config space;Yes. I forgot the variable length part.The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.Given these two disadvantages, I also prefer independent SET command the way you have it.
OK, let's wait for Michael's input again. Thanks.
2. Reserve the following structure: struct virtnet_hash_tunnel { le32 enabled_tunnel_types; }; 3. Reserve the SET command for enabled_tunnel_types and remove the GET command for enabled_tunnel_types. [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html Thanks a lot!
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]