OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash



å 2023/6/30 äå1:59, Michael S. Tsirkin åé:

On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:


å 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 AM

struct 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_VQ

along 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];
};

Oh, I forgot that rss and hash had identical structures.
And we want to keep that I think.

But now it's not clear to me: does the same enabled_tunnel_types
apply to both hash calculation and rss?
Yes. What I'm trying to say is that making the inner header hash and RSS/hash calculation clear delimitation will make everything easier. 
The inner header hash just configures enabled_tunnel_types.
The RSS/hash calculation is configured as before without modification.


I note we normally have separate configs for hash and rss.
So to keep it consistent what should we do? two set commands?
As I explained above, like outer udp port hash/symmetric toeplitz hash, these fall under the umbrella of RSS/hash calculation. 
Let's keep the inner header hash simple.


Or does enabled_tunnel_types apply to both rss and hash?


Certainly. See:
ÂÂÂ +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT. 



We should have reserved more space. We can still do it if you like:

struct virtio_net_rss_tunnel_config {
       le32 enabled_tunnel_types;
       le16 reserved[6];
       struct virtio_net_rss_config hash;
};

struct virtio_net_hash_tunnel_config {
       le32 enabled_tunnel_types;
       le16 reserved[6];
       struct virtio_net_hash_config hash;
};

?







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.

Or it could be an advantage since one normally wants to configure a
symmetric key with this. Further device can just use the new config
When we want to configure the hash key, he continues to use the previous rss/hash calculation interface. This is ok. 

Thanks.


with no need to check what the old one was. I'd call it a wash.


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.


This part is not critical to me, but now I understand we need two sets of SET commands.



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!


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]