netdev - RE: [PATCH V1 net-next 5/6] net: ena: add ethtool function for changing io queue sizes

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <45419c297d5241d9a7768b4d9af7d9f6@EX13D11EUB003.ant.amazon.com>
Date:   Mon, 10 Jun 2019 07:46:08 +0000
From:   "Jubran, Samih" <sameehj@...zon.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "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 V1 net-next 5/6] net: ena: add ethtool function for
 changing io queue sizes



> -----Original Message-----
> From: Michal Kubecek <mkubecek@...e.cz>
> Sent: Thursday, June 6, 2019 5:48 PM
> To: netdev@...r.kernel.org
> Cc: Jubran, Samih <sameehj@...zon.com>; davem@...emloft.net;
> 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 V1 net-next 5/6] net: ena: add ethtool function for
> changing io queue sizes
> 
> On Thu, Jun 06, 2019 at 02:55:19PM +0300, sameehj@...zon.com wrote:
> > From: Sameeh Jubran <sameehj@...zon.com>
> >
> > Implement the set_ringparam() function of the ethtool interface to
> > enable the changing of io queue sizes.
> >
> > Signed-off-by: Arthur Kiyanovski <akiyano@...zon.com>
> > Signed-off-by: Sameeh Jubran <sameehj@...zon.com>
> > ---
> >  drivers/net/ethernet/amazon/ena/ena_ethtool.c | 25
> > +++++++++++++++++++
> drivers/net/ethernet/amazon/ena/ena_netdev.c  |
> > 14 +++++++++++  drivers/net/ethernet/amazon/ena/ena_netdev.h  |  5
> > +++-
> >  3 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > index 101d93f16..33e28ad71 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > @@ -495,6 +495,30 @@ static void ena_get_ringparam(struct net_device
> *netdev,
> >  	ring->rx_pending = adapter->rx_ring[0].ring_size;  }
> >
> > +static int ena_set_ringparam(struct net_device *netdev,
> > +			     struct ethtool_ringparam *ring) {
> > +	struct ena_adapter *adapter = netdev_priv(netdev);
> > +	u32 new_tx_size, new_rx_size;
> > +
> > +	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
> > +		return -EINVAL;
> 
> This check is superfluous as ethtool_set_ringparam() checks supplied values
> against maximum returned by ->get_ringparam() which will be 0 in this case.
> 
> > +
> > +	new_tx_size = clamp_val(ring->tx_pending, ENA_MIN_RING_SIZE,
> > +				adapter->max_tx_ring_size);
> > +	new_tx_size = rounddown_pow_of_two(new_tx_size);
> > +
> > +	new_rx_size = clamp_val(ring->rx_pending, ENA_MIN_RING_SIZE,
> > +				adapter->max_rx_ring_size);
> > +	new_rx_size = rounddown_pow_of_two(new_rx_size);
> 
> For the same reason, clamping from below would suffice here.
> 
> Michal Kubecek
> 
> > +
> > +	if (new_tx_size == adapter->requested_tx_ring_size &&
> > +	    new_rx_size == adapter->requested_rx_ring_size)
> > +		return 0;
> > +
> > +	return ena_update_queue_sizes(adapter, new_tx_size,
> new_rx_size); }

You are right with both arguments the way the code is written now, however, in the future the code might change and we prefer to be extra cautious.