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

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

å 2023/2/21 äå11:32, Parav Pandit åé:
> > From: Heng Qi <hengqi@linux.alibaba.com>
> > Sent: Tuesday, February 21, 2023 8:34 AM
> > > Even without a queue overflow, this shared receive queue may not have a
> > balanced number of packets.
> > > For example, tunnel-2 occupied 90% of the queue and left only 10% for
> > tunnel-1.
> > > So, your example is right (and extreme), a generic mention of QoS covers
> > both aspects.
> > > Secondly "user inside the tunnel" is challenging to explain.
> > > In above sentence talks specifically about the "receive queue" as an object.
> > "overflow" in my example. If we only use a one-sentence summary to describe
> > tunnel risks, then I think this subparagraph is called "QoS problem for tunnel
> > hash".
> Yes, Tunnel Qos limitations
>
> > > > 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 so, virtio_net_hash_config does not require hash_tunnel_types when
> > > > it does not need to configure the specific tunnel(s).
> > > >
> > > > > If the intent is to enable hashing for the specific tunnel(s), an
> > > > > individual
> > > > command is better.
> > > >
> > > > Drivers MAY filter out some tunneling types when negotiating this feature.
> > > > Do you think it would be better for us to add a separate command? I
> > > > don't see tools like ethtool that can configure specific tunnels in userspace.
> > > The reason I proposed different command is, Let's say we have only
> > > single command.
> > > Rss config command has many other fields unrelated to the inner hash.
> > Yes. For inner hash, fields such as indirection table are irrelevant.
> Right.
>
> > > Hence, to enable inner hash, the driver needs to supply the exact same value
> > for unrelated fields to the same value.
> > > And device needs to compare it with the old value and maintain some sort of
> > cache to derive that nothing changes from the previous hash config, hence,
> > ignore hw configuration for rss.
> > > This mechanism slows down the command for the unrelated task.
> > Totally agree.
> >
> > > Hence, I am considering a separate command that would be simple for the
> > device and driver to implement.
> >
> > I agree with you. Do you want me to do it in this patch, or should we do it in
> > another patch?
> It should be in this patch set because enablement in the device is linked to this capability exposed in this patch.
>
>  From patch split point of view,
>
> Patch-1 to introduce the feature bit, description, and link to CVQ dependency.
> Patch-2 for its link in virtio_net_config structure and description.
> Patch-3 for new command touching control VQ pieces.

Yes, and you seem to have missed my other replies in this thread:), I 
rephrased it here:
virtio_net_hash_config seems to be reusable, as the v9 patch is doing, 
why don't we re-use it?
The reason I can think of is to not expand the virtio_net_hash_config 
structure, just set a separate
structure for VIRTIO_NET_F_HASH_TUNNEL and include hash_tunnel_types?

Thanks.
> We can always squash the patch to single when/if it is hard to understand in multiple patches.