| Message ID | 20190606115520.20394-6-sameehj@amazon.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Support for dynamic queue size changes | expand |
On Thu, Jun 06, 2019 at 02:55:19PM +0300, sameehj@amazon.com wrote: > From: Sameeh Jubran <sameehj@amazon.com> > > Implement the set_ringparam() function of the ethtool interface > to enable the changing of io queue sizes. > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com> > Signed-off-by: Sameeh Jubran <sameehj@amazon.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); > +}
On Thu, Jun 06, 2019 at 02:55:19PM +0300, sameehj@amazon.com wrote: > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c > index 938aca254..7d3837c13 100644 > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c > @@ -2031,6 +2031,20 @@ static int ena_close(struct net_device *netdev) > return 0; > } > > +int ena_update_queue_sizes(struct ena_adapter *adapter, > + int new_tx_size, > + int new_rx_size) > +{ > + bool dev_up; > + > + dev_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; > +} This function is called with u32 values as arguments by its only caller and copies them into u32 members of struct ena_adapter. Why are its arguments new_tx_size and new_rx_size declared as int? Michal Kubecek
> -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Thursday, June 6, 2019 5:48 PM > To: netdev@vger.kernel.org > Cc: Jubran, Samih <sameehj@amazon.com>; davem@davemloft.net; > 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 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@amazon.com wrote: > > From: Sameeh Jubran <sameehj@amazon.com> > > > > Implement the set_ringparam() function of the ethtool interface to > > enable the changing of io queue sizes. > > > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com> > > Signed-off-by: Sameeh Jubran <sameehj@amazon.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.
On Mon, Jun 10, 2019 at 07:46:08AM +0000, Jubran, Samih wrote: > > -----Original Message----- > > From: Michal Kubecek <mkubecek@suse.cz> > > Sent: Thursday, June 6, 2019 5:48 PM > > To: netdev@vger.kernel.org > > Cc: Jubran, Samih <sameehj@amazon.com>; davem@davemloft.net; > > 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 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@amazon.com wrote: > > > From: Sameeh Jubran <sameehj@amazon.com> > > > > > > Implement the set_ringparam() function of the ethtool interface to > > > enable the changing of io queue sizes. > > > > > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com> > > > Signed-off-by: Sameeh Jubran <sameehj@amazon.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. If we accept this logic, commit 37e2d99b59c4 ("ethtool: Ensure new ring parameters are within bounds during SRINGPARAM") would be useless as every driver would have to duplicate the checks anyway. Michal Kubecek
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; + + 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); + + 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); +} + static u32 ena_flow_hash_to_flow_type(u16 hash_fields) { u32 data = 0; @@ -860,6 +884,7 @@ static const struct ethtool_ops ena_ethtool_ops = { .get_coalesce = ena_get_coalesce, .set_coalesce = ena_set_coalesce, .get_ringparam = ena_get_ringparam, + .set_ringparam = ena_set_ringparam, .get_sset_count = ena_get_sset_count, .get_strings = ena_get_strings, .get_ethtool_stats = ena_get_ethtool_stats, diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c index 938aca254..7d3837c13 100644 --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c @@ -2031,6 +2031,20 @@ static int ena_close(struct net_device *netdev) return 0; } +int ena_update_queue_sizes(struct ena_adapter *adapter, + int new_tx_size, + int new_rx_size) +{ + bool dev_up; + + dev_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; +} + static void ena_tx_csum(struct ena_com_tx_ctx *ena_tx_ctx, struct sk_buff *skb) { u32 mss = skb_shinfo(skb)->gso_size; diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h index e8fe08cb7..2da20ad5a 100644 --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h @@ -81,7 +81,6 @@ #define ENA_DEFAULT_RING_SIZE (1024) #define ENA_MIN_RING_SIZE (256) - #define ENA_TX_WAKEUP_THRESH (MAX_SKB_FRAGS + 2) #define ENA_DEFAULT_RX_COPYBREAK (256 - NET_IP_ALIGN) @@ -389,6 +388,10 @@ void ena_dump_stats_to_dmesg(struct ena_adapter *adapter); void ena_dump_stats_to_buf(struct ena_adapter *adapter, u8 *buf); +int ena_update_queue_sizes(struct ena_adapter *adapter, + int new_tx_size, + int new_rx_size); + int ena_get_sset_count(struct net_device *netdev, int sset); #endif /* !(ENA_H) */