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

[Parav Pandit <parav@nvidia.com>]

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

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