From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suzuki Shota Date: Thu, 23 Jul 2015 14:56:44 +0900 Subject: [Intel-wired-lan] [PATCH net] igb: Fix oops caused by missing queue pairing In-Reply-To: <1435710352-956-1-git-send-email-suzuki_shota_t3@lab.ntt.co.jp> References: <1435710352-956-1-git-send-email-suzuki_shota_t3@lab.ntt.co.jp> Message-ID: <55B0821C.4060809@lab.ntt.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: 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 > --- > 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: >