Re: [PATCH v3 4/4] drivers/net/virtio_net: Added RSS hash report control.

[Andrew Melnichenko]

Hi all,


On Tue, Feb 8, 2022 at 10:59 PM Willem de Bruijn
<willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> On Tue, Feb 8, 2022 at 1:19 PM Andrew Melnychenko <andrew@xxxxxxxxxx> wrote:
> >
> > Now it's possible to control supported hashflows.
> > Added hashflow set/get callbacks.
> > Also, disabling RXH_IP_SRC/DST for TCP would disable then for UDP.
>
> I don't follow this comment. Can you elaborate?

I'll rephrase it in next version of patches.
The idea is that VirtioNet RSS doesn't distinguish IP hashes between
TCP and UDP.
For TCP and UDP it's possible to set IP+PORT hashes.
But disabling IP hashes will disable them for TCP and UDP simultaneously.
It's possible to set IP+PORT for TCP and IP for everything else(UDP, ICMP etc.)

>
> > TCP and UDP supports only:
> > ethtool -U eth0 rx-flow-hash tcp4 sd
> >     RXH_IP_SRC + RXH_IP_DST
> > ethtool -U eth0 rx-flow-hash tcp4 sdfn
> >     RXH_IP_SRC + RXH_IP_DST + RXH_L4_B_0_1 + RXH_L4_B_2_3
> >
> > Signed-off-by: Andrew Melnychenko <andrew@xxxxxxxxxx>
> > ---
> >  drivers/net/virtio_net.c | 141 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 140 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 543da2fbdd2d..88759d5e693c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -231,6 +231,7 @@ struct virtnet_info {
> >         u8 rss_key_size;
> >         u16 rss_indir_table_size;
> >         u32 rss_hash_types_supported;
> > +       u32 rss_hash_types_saved;
>
> hash_types_active?

I think "hash_types_saved" is more suitable for the current field.
Idea is that the user may disable RSS/HASH and we need to save
what hash type configurations previously were enabled.
So, we can restore it when the user will enable RSS/HASH back.

>
> > +static bool virtnet_set_hashflow(struct virtnet_info *vi, struct ethtool_rxnfc *info)
> > +{
> > +       u32 new_hashtypes = vi->rss_hash_types_saved;
> > +       bool is_disable = info->data & RXH_DISCARD;
> > +       bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
> > +
> > +       /* supports only 'sd', 'sdfn' and 'r' */
> > +       if (!((info->data == (RXH_IP_SRC | RXH_IP_DST)) | is_l4 | is_disable))
>
> maybe add an is_l3

There used to be "is_l3", but that variable was used only in that
condition statement.
So I've decided to inplace it.

>
> > +               return false;
> > +
> > +       switch (info->flow_type) {
> > +       case TCP_V4_FLOW:
> > +               new_hashtypes &= ~(VIRTIO_NET_RSS_HASH_TYPE_IPv4 | VIRTIO_NET_RSS_HASH_TYPE_TCPv4);
> > +               if (!is_disable)
> > +                       new_hashtypes |= VIRTIO_NET_RSS_HASH_TYPE_IPv4
> > +                               | (is_l4 ? VIRTIO_NET_RSS_HASH_TYPE_TCPv4 : 0);
> > +               break;
> > +       case UDP_V4_FLOW:
> > +               new_hashtypes &= ~(VIRTIO_NET_RSS_HASH_TYPE_IPv4 | VIRTIO_NET_RSS_HASH_TYPE_UDPv4);
> > +               if (!is_disable)
> > +                       new_hashtypes |= VIRTIO_NET_RSS_HASH_TYPE_IPv4
> > +                               | (is_l4 ? VIRTIO_NET_RSS_HASH_TYPE_UDPv4 : 0);
> > +               break;
> > +       case IPV4_FLOW:
> > +               new_hashtypes &= ~VIRTIO_NET_RSS_HASH_TYPE_IPv4;
> > +               if (!is_disable)
> > +                       new_hashtypes = VIRTIO_NET_RSS_HASH_TYPE_IPv4;
> > +               break;
> > +       case TCP_V6_FLOW:
> > +               new_hashtypes &= ~(VIRTIO_NET_RSS_HASH_TYPE_IPv6 | VIRTIO_NET_RSS_HASH_TYPE_TCPv6);
> > +               if (!is_disable)
> > +                       new_hashtypes |= VIRTIO_NET_RSS_HASH_TYPE_IPv6
> > +                               | (is_l4 ? VIRTIO_NET_RSS_HASH_TYPE_TCPv6 : 0);
> > +               break;
> > +       case UDP_V6_FLOW:
> > +               new_hashtypes &= ~(VIRTIO_NET_RSS_HASH_TYPE_IPv6 | VIRTIO_NET_RSS_HASH_TYPE_UDPv6);
> > +               if (!is_disable)
> > +                       new_hashtypes |= VIRTIO_NET_RSS_HASH_TYPE_IPv6
> > +                               | (is_l4 ? VIRTIO_NET_RSS_HASH_TYPE_UDPv6 : 0);
> > +               break;
> > +       case IPV6_FLOW:
> > +               new_hashtypes &= ~VIRTIO_NET_RSS_HASH_TYPE_IPv6;
> > +               if (!is_disable)
> > +                       new_hashtypes = VIRTIO_NET_RSS_HASH_TYPE_IPv6;
> > +               break;
> > +       default:
> > +               /* unsupported flow */
> > +               return false;
> > +       }
> > +
> > +       /* if unsupported hashtype was set */
> > +       if (new_hashtypes != (new_hashtypes & vi->rss_hash_types_supported))
> > +               return false;
> > +
> > +       if (new_hashtypes != vi->rss_hash_types_saved) {
> > +               vi->rss_hash_types_saved = new_hashtypes;
>
> should only be updated if the commit function returned success?

Not really, we already made all checks against "supported" hash types.
Also, the commit function may not be called if RSS is disabled by the user.