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

["Michael S. Tsirkin" <mst@redhat.com>]

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?
I note we normally have separate configs for hash and rss.
So to keep it consistent what should we do? two set commands?
Or does enabled_tunnel_types apply to both rss and hash?

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
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!