| 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
| ||
|
Message-ID: <577acdeedc7a4db6a28b5e8350e7b88f@EX13D11EUB003.ant.amazon.com>
Date: Thu, 3 Oct 2019 15:32:33 +0000
From: "Jubran, Samih" <sameehj@...zon.com>
To: Jakub Kicinski <jakub.kicinski@...ronome.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
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Sent: Wednesday, October 2, 2019 11:12 PM
> To: Jubran, Samih <sameehj@...zon.com>
> Cc: davem@...emloft.net; 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 Wed, 2 Oct 2019 11:20:52 +0300, sameehj@...zon.com wrote:
> > From: Sameeh Jubran <sameehj@...zon.com>
> >
> > Set channels callback enables the user to change the count of queues
> > used by the driver using ethtool. We decided to currently support only
> > equal number of rx and tx queues, this might change in the future.
> >
> > Also rename dev_up to dev_was_up in ena_update_queue_count() to
> make
> > it clearer.
> >
> > Signed-off-by: Sameeh Jubran <sameehj@...zon.com>
> > ---
> > drivers/net/ethernet/amazon/ena/ena_ethtool.c | 17 ++++++++++++++
> > drivers/net/ethernet/amazon/ena/ena_netdev.c | 22
> ++++++++++++++++---
> > drivers/net/ethernet/amazon/ena/ena_netdev.h | 3 +++
> > 3 files changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > index c9d760465..f58fc3c68 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > @@ -744,6 +744,22 @@ static void ena_get_channels(struct net_device
> *netdev,
> > channels->combined_count = 0;
> > }
> >
> > +static int ena_set_channels(struct net_device *netdev,
> > + struct ethtool_channels *channels) {
> > + struct ena_adapter *adapter = netdev_priv(netdev);
> > + u32 new_channel_count;
> > +
> > + if (channels->rx_count != channels->tx_count ||
>
> If you use the same IRQ and NAPI to service RX and TX it's a combined
> channel, not rx and tx channels.
Will do.
>
> > + channels->max_tx != channels->max_rx)
>
> Hm.. maxes are generally ignored on set 🤔
>
> > + return -EINVAL;
> > +
> > + new_channel_count = clamp_val(channels->tx_count,
> > + ENA_MIN_NUM_IO_QUEUES, channels-
> >max_tx);
>
> You should return an error if the value is not within bounds, rather than
> guessing.
Will do.
>
> > + return ena_update_queue_count(adapter, new_channel_count); }
> > +
> > static int ena_get_tunable(struct net_device *netdev,
> > const struct ethtool_tunable *tuna, void *data) {
> @@ -807,6
> > +823,7 @@ static const struct ethtool_ops ena_ethtool_ops = {
> > .get_rxfh = ena_get_rxfh,
> > .set_rxfh = ena_set_rxfh,
> > .get_channels = ena_get_channels,
> > + .set_channels = ena_set_channels,
> > .get_tunable = ena_get_tunable,
> > .set_tunable = ena_set_tunable,
> > };
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > index e964783c4..7d44b3440 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > @@ -2044,14 +2044,30 @@ int ena_update_queue_sizes(struct
> ena_adapter *adapter,
> > u32 new_tx_size,
> > u32 new_rx_size)
> > {
> > - bool dev_up;
> > + bool dev_was_up;
> >
> > - dev_up = test_bit(ENA_FLAG_DEV_UP, &adapter->flags);
> > + dev_was_up = test_bit(ENA_FLAG_DEV_UP, &adapter->flags);
> > ena_close(adapter->netdev);
> > adapter->requested_tx_ring_size = new_tx_size;
> > adapter->requested_rx_ring_size = new_rx_size;
> > ena_init_io_rings(adapter);
> > - return dev_up ? ena_up(adapter) : 0;
> > + return dev_was_up ? ena_up(adapter) : 0; }
> > +
> > +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()
> > + */
> > + 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.
>
> 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)
Powered by blists - more mailing lists