netdev - Re: [PATCH net-next] gve: Add RSS cache for non RSS device option scenario

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG-FcCP7iJaVwWXwaCPN_N83RMe2Tb5sahhnf6YxRU48gxE1gQ@mail.gmail.com>
Date: Tue, 21 Jan 2025 10:05:44 -0800
From: Ziwei Xiao <ziweixiao@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Praveen Kaligineedi <pkaligineedi@...gle.com>, netdev@...r.kernel.org, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com, shailend@...gle.com, 
	andrew+netdev@...n.ch, willemb@...gle.com, hramamurthy@...gle.com, 
	horms@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] gve: Add RSS cache for non RSS device option scenario

On Tue, Jan 14, 2025 at 6:02 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 13 Jan 2025 13:11:39 -0800 Praveen Kaligineedi wrote:
> >  static int gve_set_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh,
> >                       struct netlink_ext_ack *extack)
> >  {
> >       struct gve_priv *priv = netdev_priv(netdev);
> > +     int err;
> >
> >       if (!priv->rss_key_size || !priv->rss_lut_size)
> >               return -EOPNOTSUPP;
> >
> > -     return gve_adminq_configure_rss(priv, rxfh);
> > +     if (priv->cache_rss_config && !priv->rss_cache_configured) {
> > +             err = gve_init_rss_config(priv);
>
> I don't understand why this is here.
> Why are you programming the default config the first time user asks
> to set rxfh? And just to immediately overwrite it.

>
> Shouldn't you be doing this on probe or first open?
Thank you for pointing this out. I will send V2 patch when the window
is open to make it initialize RSS when the driver probe, and remove
the gve_init_rss_config code in the gve_set_rxfh.
>
> Oh and in gve_setup_device_resources() you call gve_init_rss_config()
>
> > +     if (priv->rss_cache_configured) {
>
> so you reset what user wanted to defaults, only if user wanted
> something explicitly _not_ default. Hm.
>
This if check will be removed in the V2 patch too.
> > +             if (err) {
> > +                     dev_err(&priv->pdev->dev, "Fail to init RSS config\n");
>
> use extack, please
>
Will be updated in the V2 patch
> > +                     return err;
> > +             }
> > +     }
>
> > +int gve_init_rss_config(struct gve_priv *priv)
> > +{
> > +     struct gve_rss_config *rss_config = &priv->rss_config;
> > +     struct ethtool_rxfh_param rxfh;
> > +     u16 i;
> > +
> > +     for (i = 0; i < priv->rss_lut_size; i++)
> > +             rss_config->hash_lut[i] = i % priv->rx_cfg.num_queues;
>
> nit: ethtool_rxfh_indir_default()
>
Will be updated in the V2 patch
> > +
> > +     netdev_rss_key_fill(rss_config->hash_key, priv->rss_key_size);
> > +
> > +     rxfh.hfunc = ETH_RSS_HASH_TOP;
> > +
> > +     return gve_adminq_configure_rss(priv, &rxfh);
> > +}
> --
> pw-bot: cr