| Message ID | 20191002082052.14051-6-sameehj@amazon.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Introduce ethtool's set_channels | expand |
On Wed, 2 Oct 2019 11:20:52 +0300, sameehj@amazon.com wrote: > From: Sameeh Jubran <sameehj@amazon.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@amazon.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. > + 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. > + 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. 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)
> -----Original Message----- > From: Jakub Kicinski <jakub.kicinski@netronome.com> > Sent: Wednesday, October 2, 2019 11:12 PM > To: Jubran, Samih <sameehj@amazon.com> > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>; > Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed > <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori, > Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>; > Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel > <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt, > Benjamin <benh@amazon.com>; Kiyanovski, Arthur > <akiyano@amazon.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@amazon.com wrote: > > From: Sameeh Jubran <sameehj@amazon.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@amazon.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)
On Wed, Oct 02, 2019 at 01:11:32PM -0700, Jakub Kicinski wrote: > On Wed, 2 Oct 2019 11:20:52 +0300, sameehj@amazon.com wrote: > > + > > + 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. And ethtool_set_channels() already does that if any of the requested counts exceeds the corresponding maximum so that the upper bound check here is superfluous. Michal
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)
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 || + channels->max_tx != channels->max_rx) + return -EINVAL; + + new_channel_count = clamp_val(channels->tx_count, + ENA_MIN_NUM_IO_QUEUES, channels->max_tx); + + 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; } static void ena_tx_csum(struct ena_com_tx_ctx *ena_tx_ctx, struct sk_buff *skb) diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h index 7499afb58..bffd778f2 100644 --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h @@ -82,6 +82,8 @@ #define ENA_DEFAULT_RING_SIZE (1024) #define ENA_MIN_RING_SIZE (256) +#define ENA_MIN_NUM_IO_QUEUES (1) + #define ENA_TX_WAKEUP_THRESH (MAX_SKB_FRAGS + 2) #define ENA_DEFAULT_RX_COPYBREAK (256 - NET_IP_ALIGN) @@ -388,6 +390,7 @@ void ena_dump_stats_to_buf(struct ena_adapter *adapter, u8 *buf); int ena_update_queue_sizes(struct ena_adapter *adapter, u32 new_tx_size, u32 new_rx_size); +int ena_update_queue_count(struct ena_adapter *adapter, u32 new_channel_count); int ena_get_sset_count(struct net_device *netdev, int sset);