| Message ID | 1435710352-956-1-git-send-email-suzuki_shota_t3@lab.ntt.co.jp |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
Dear Jeffrey, With regard to the patch I submitted, would you tell me the review status of it? (I'm watching Patchwork of intel-wired-lan, but the state of my patch is still 'New') Do I have to do other things? Regards, Shota Suzuki On 2015/07/01 9:25, Shota Suzuki wrote: > When initializing igb driver (e.g. 82576, I350), IGB_FLAG_QUEUE_PAIRS is > set if adapter->rss_queues exceeds half of max_rss_queues in > igb_init_queue_configuration(). > On the other hand, IGB_FLAG_QUEUE_PAIRS is not set even if the number of > queues exceeds half of max_combined in igb_set_channels() when changing > the number of queues by "ethtool -L". > In this case, if numvecs is larger than MAX_MSIX_ENTRIES (10), the size > of adapter->msix_entries[], an overflow can occur in > igb_set_interrupt_capability(), which in turn leads to an oops. > > Fix this problem as follows: > - When changing the number of queues by "ethtool -L", set > IGB_FLAG_QUEUE_PAIRS in the same way as initializing igb driver. > - When increasing the size of q_vector, reallocate it appropriately. > (With IGB_FLAG_QUEUE_PAIRS set, the size of q_vector gets larger.) > > Another possible way to fix this problem is to cap the queues at its > initial number, which is the number of the initial online cpus. But this > is not the optimal way because we cannnot increase queues when another > cpu becomes online. > > Note that before commit cd14ef54d25b ("igb: Change to use statically > allocated array for MSIx entries"), this problem did not cause oops > but just made the number of queues become 1 because of entering msi_only > mode in igb_set_interrupt_capability(). > > Fixes: 907b7835799f ("igb: Add ethtool support to configure number of channels") > Signed-off-by: Shota Suzuki <suzuki_shota_t3@lab.ntt.co.jp> > --- > Although we might be able to additionally unset IGB_FLAG_QUEUE_PAIRS > when it is not needed, this patch doesn't change existing behaviour > because such a change is not a bug fix. > > drivers/net/ethernet/intel/igb/igb.h | 1 + > drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 ++++- > drivers/net/ethernet/intel/igb/igb_main.c | 16 ++++++++++++++-- > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h > index c2bd4f9..212d668 100644 > --- a/drivers/net/ethernet/intel/igb/igb.h > +++ b/drivers/net/ethernet/intel/igb/igb.h > @@ -540,6 +540,7 @@ void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, unsigned char *va, > struct sk_buff *skb); > int igb_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr); > int igb_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr); > +void igb_set_flag_queue_pairs(struct igb_adapter *, const u32); > #ifdef CONFIG_IGB_HWMON > void igb_sysfs_exit(struct igb_adapter *adapter); > int igb_sysfs_init(struct igb_adapter *adapter); > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c > index d5673eb..0afc091 100644 > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c > @@ -2991,6 +2991,7 @@ static int igb_set_channels(struct net_device *netdev, > { > struct igb_adapter *adapter = netdev_priv(netdev); > unsigned int count = ch->combined_count; > + unsigned int max_combined = 0; > > /* Verify they are not requesting separate vectors */ > if (!count || ch->rx_count || ch->tx_count) > @@ -3001,11 +3002,13 @@ static int igb_set_channels(struct net_device *netdev, > return -EINVAL; > > /* Verify the number of channels doesn't exceed hw limits */ > - if (count > igb_max_channels(adapter)) > + max_combined = igb_max_channels(adapter); > + if (count > max_combined) > return -EINVAL; > > if (count != adapter->rss_queues) { > adapter->rss_queues = count; > + igb_set_flag_queue_pairs(adapter, max_combined); > > /* Hardware has to reinitialize queues and interrupts to > * match the new configuration. > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index f287186..59dc2b4 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -1205,10 +1205,14 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter, > > /* allocate q_vector and rings */ > q_vector = adapter->q_vector[v_idx]; > - if (!q_vector) > + if (!q_vector) { > q_vector = kzalloc(size, GFP_KERNEL); > - else > + } else if (size > ksize(q_vector)) { > + kfree_rcu(q_vector, rcu); > + q_vector = kzalloc(size, GFP_KERNEL); > + } else { > memset(q_vector, 0, size); > + } > if (!q_vector) > return -ENOMEM; > > @@ -2888,6 +2892,14 @@ static void igb_init_queue_configuration(struct igb_adapter *adapter) > > adapter->rss_queues = min_t(u32, max_rss_queues, num_online_cpus()); > > + igb_set_flag_queue_pairs(adapter, max_rss_queues); > +} > + > +void igb_set_flag_queue_pairs(struct igb_adapter *adapter, > + const u32 max_rss_queues) > +{ > + struct e1000_hw *hw = &adapter->hw; > + > /* Determine if we need to pair queues. */ > switch (hw->mac.type) { > case e1000_82575: >
On Thu, 2015-07-23 at 14:56 +0900, Suzuki Shota wrote: > With regard to the patch I submitted, > would you tell me the review status of it? > (I'm watching Patchwork of intel-wired-lan, > but the state of my patch is still 'New') > > Do I have to do other things? Nope, you do not have to do anything. I just got back-logged with work and I am processing patches currently. Sorry for the delay.
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Shota Suzuki > Sent: Tuesday, June 30, 2015 5:26 PM > To: Kirsher, Jeffrey T; Brandeburg, Jesse; Nelson, Shannon; Wyborny, > Carolyn; Skidmore, Donald C; Vick, Matthew; Ronciak, John; Williams, Mitch > A; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org > Cc: Shota Suzuki > Subject: [PATCH net] igb: Fix oops caused by missing queue pairing > > When initializing igb driver (e.g. 82576, I350), IGB_FLAG_QUEUE_PAIRS is > set if adapter->rss_queues exceeds half of max_rss_queues in > igb_init_queue_configuration(). > On the other hand, IGB_FLAG_QUEUE_PAIRS is not set even if the number of > queues exceeds half of max_combined in igb_set_channels() when changing > the number of queues by "ethtool -L". > In this case, if numvecs is larger than MAX_MSIX_ENTRIES (10), the size > of adapter->msix_entries[], an overflow can occur in > igb_set_interrupt_capability(), which in turn leads to an oops. > > Fix this problem as follows: > - When changing the number of queues by "ethtool -L", set > IGB_FLAG_QUEUE_PAIRS in the same way as initializing igb driver. > - When increasing the size of q_vector, reallocate it appropriately. > (With IGB_FLAG_QUEUE_PAIRS set, the size of q_vector gets larger.) > > Another possible way to fix this problem is to cap the queues at its > initial number, which is the number of the initial online cpus. But this > is not the optimal way because we cannnot increase queues when another > cpu becomes online. > > Note that before commit cd14ef54d25b ("igb: Change to use statically > allocated array for MSIx entries"), this problem did not cause oops > but just made the number of queues become 1 because of entering msi_only > mode in igb_set_interrupt_capability(). > > Fixes: 907b7835799f ("igb: Add ethtool support to configure number of > channels") > Signed-off-by: Shota Suzuki <suzuki_shota_t3@lab.ntt.co.jp> > --- > Although we might be able to additionally unset IGB_FLAG_QUEUE_PAIRS > when it is not needed, this patch doesn't change existing behaviour > because such a change is not a bug fix. > > drivers/net/ethernet/intel/igb/igb.h | 1 + > drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 ++++- > drivers/net/ethernet/intel/igb/igb_main.c | 16 ++++++++++++++-- > 3 files changed, 19 insertions(+), 3 deletions(-) Tested-by: Aaron Brown <aaron.f.brown@intel.com>
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h index c2bd4f9..212d668 100644 --- a/drivers/net/ethernet/intel/igb/igb.h +++ b/drivers/net/ethernet/intel/igb/igb.h @@ -540,6 +540,7 @@ void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, unsigned char *va, struct sk_buff *skb); int igb_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr); int igb_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr); +void igb_set_flag_queue_pairs(struct igb_adapter *, const u32); #ifdef CONFIG_IGB_HWMON void igb_sysfs_exit(struct igb_adapter *adapter); int igb_sysfs_init(struct igb_adapter *adapter); diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c index d5673eb..0afc091 100644 --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c @@ -2991,6 +2991,7 @@ static int igb_set_channels(struct net_device *netdev, { struct igb_adapter *adapter = netdev_priv(netdev); unsigned int count = ch->combined_count; + unsigned int max_combined = 0; /* Verify they are not requesting separate vectors */ if (!count || ch->rx_count || ch->tx_count) @@ -3001,11 +3002,13 @@ static int igb_set_channels(struct net_device *netdev, return -EINVAL; /* Verify the number of channels doesn't exceed hw limits */ - if (count > igb_max_channels(adapter)) + max_combined = igb_max_channels(adapter); + if (count > max_combined) return -EINVAL; if (count != adapter->rss_queues) { adapter->rss_queues = count; + igb_set_flag_queue_pairs(adapter, max_combined); /* Hardware has to reinitialize queues and interrupts to * match the new configuration. diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index f287186..59dc2b4 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -1205,10 +1205,14 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter, /* allocate q_vector and rings */ q_vector = adapter->q_vector[v_idx]; - if (!q_vector) + if (!q_vector) { q_vector = kzalloc(size, GFP_KERNEL); - else + } else if (size > ksize(q_vector)) { + kfree_rcu(q_vector, rcu); + q_vector = kzalloc(size, GFP_KERNEL); + } else { memset(q_vector, 0, size); + } if (!q_vector) return -ENOMEM; @@ -2888,6 +2892,14 @@ static void igb_init_queue_configuration(struct igb_adapter *adapter) adapter->rss_queues = min_t(u32, max_rss_queues, num_online_cpus()); + igb_set_flag_queue_pairs(adapter, max_rss_queues); +} + +void igb_set_flag_queue_pairs(struct igb_adapter *adapter, + const u32 max_rss_queues) +{ + struct e1000_hw *hw = &adapter->hw; + /* Determine if we need to pair queues. */ switch (hw->mac.type) { case e1000_82575:
When initializing igb driver (e.g. 82576, I350), IGB_FLAG_QUEUE_PAIRS is set if adapter->rss_queues exceeds half of max_rss_queues in igb_init_queue_configuration(). On the other hand, IGB_FLAG_QUEUE_PAIRS is not set even if the number of queues exceeds half of max_combined in igb_set_channels() when changing the number of queues by "ethtool -L". In this case, if numvecs is larger than MAX_MSIX_ENTRIES (10), the size of adapter->msix_entries[], an overflow can occur in igb_set_interrupt_capability(), which in turn leads to an oops. Fix this problem as follows: - When changing the number of queues by "ethtool -L", set IGB_FLAG_QUEUE_PAIRS in the same way as initializing igb driver. - When increasing the size of q_vector, reallocate it appropriately. (With IGB_FLAG_QUEUE_PAIRS set, the size of q_vector gets larger.) Another possible way to fix this problem is to cap the queues at its initial number, which is the number of the initial online cpus. But this is not the optimal way because we cannnot increase queues when another cpu becomes online. Note that before commit cd14ef54d25b ("igb: Change to use statically allocated array for MSIx entries"), this problem did not cause oops but just made the number of queues become 1 because of entering msi_only mode in igb_set_interrupt_capability(). Fixes: 907b7835799f ("igb: Add ethtool support to configure number of channels") Signed-off-by: Shota Suzuki <suzuki_shota_t3@lab.ntt.co.jp> --- Although we might be able to additionally unset IGB_FLAG_QUEUE_PAIRS when it is not needed, this patch doesn't change existing behaviour because such a change is not a bug fix. drivers/net/ethernet/intel/igb/igb.h | 1 + drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 ++++- drivers/net/ethernet/intel/igb/igb_main.c | 16 ++++++++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-)