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-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash



å 2023/6/30 äå4:17, Michael S. Tsirkin åé:

On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:


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

Yes but note that symmetric toeplitz hash has to be configured
separately for RSS and for hashing.
Yes, this requires a field like \field{mode}, with different values corresponding to different hashing algorithms, such as toeplitz or symmetric toeplitz. 
The outer port hash belongs to 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.


It does not really say that.
Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is applied to hash and RSS. Yes. When one wants to configure inner header hash separately, use VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types separately. When one wants to configure both inner header hash and RSS/hash, use VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG. The inner header hash is decoupled from RSS/hash, and no extra fields will be sent every configuration. 






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.

I don't understand this sentence. My point is simply that
to use the tunnel key has to be symmetric. So two commands
will be required: one to set tunnel types, one to
set the key.
Yes, you are right. Then one should be VIRTIO_NET_CTRL_HASH_TUNNEL_SET and the other 
should be VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.

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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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