netdev - Re: [PATCH V2 net-next 5/5] net: ena: ethtool: support set

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]

Date:   Thu, 3 Oct 2019 10:08:19 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     "Jubran, Samih" <sameehj@...zon.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Machulsky, Zorik" <zorik@...zon.com>,
        "Matushevsky, Alexander" <matua@...zon.com>,
        "Bshara, Saeed" <saeedb@...zon.com>,
        "Wilson, Matt" <msw@...zon.com>,
        "Liguori, Anthony" <aliguori@...zon.com>,
        "Bshara, Nafea" <nafea@...zon.com>,
        "Tzalik, Guy" <gtzalik@...zon.com>,
        "Belgazal, Netanel" <netanel@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Herrenschmidt, Benjamin" <benh@...zon.com>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>
Subject: Re: [PATCH V2 net-next 5/5] net: ena: ethtool: support set_channels
 callback

On Thu, 3 Oct 2019 15:32:33 +0000, Jubran, Samih wrote:
> > > +int ena_update_queue_count(struct ena_adapter *adapter, u32 new_channel_count) {
> > > +	struct ena_com_dev *ena_dev = adapter->ena_dev;
> > > +	bool dev_was_up;
> > > +
> > > +	dev_was_up = test_bit(ENA_FLAG_DEV_UP, &adapter->flags);
> > > +	ena_close(adapter->netdev);
> > > +	adapter->num_io_queues = new_channel_count;
> > > +       /* We need to destroy the rss table so that the indirection
> > > +	* table will be reinitialized by ena_up()
> > > +	*/

Ugh, this comment is broken in the same way the comment Colin fixed in
commit 4208966f65f5 ("net: ena: clean up indentation issue") was.
Please double check your submission for this and use checkpatch.

> > > +	ena_com_rss_destroy(ena_dev);
> > > +	ena_init_io_rings(adapter);
> > > +	return dev_was_up ? ena_open(adapter->netdev) : 0;  
> > 
> > You should try to prepare the resources for the new configuration before
> > you attempt the change. Otherwise if allocation of new rings fails the open
> > will leave the device in a broken state.  
> 
> Our ena_up() applies logarithmic backoff mechanism on the rings size if
> the allocations fail, and therefore the device will stay functional.

Sure, a heuristic like that will help, but doesn't give hard
guarantees, which is what engineers like :)

> > This is not always enforced upstream, but you can see mlx5 or nfp for
> > examples of drivers which do this right..
> >   
> > >  }
> > >
> > >  static void ena_tx_csum(struct ena_com_tx_ctx *ena_tx_ctx, struct
> > > sk_buff *skb)