On Fri, 22 Oct 2010, Andy Cress wrote:
Hi Andy, > This patch does need refining. > There was one call to smp_processor_id() and one call to get_cpu(), so > something needs to be changed for at least the smp_processor_id() case > below. But in the case of smp_processor_id(), we are running under the transmit queue lock [1] which is exclusive across the machine, and on top of that even if we weren't under lock, we would be able to continue just fine even if we had been preempted because worst case we would just pick the wrong transmit queue for this transmit and then the frame would go out anyway. [1] - http://lxr.linux.no/#linux+v2.6.36/net/core/dev.c#L2211 > But when get_cpu() does the preempt_disable(), where does the > preempt_enable get invoked? from the put_cpu(). I think because we are unable to reproduce this with the upstream kernel even with DEBUG_PREEMPT set, you should probably take the issue up with windriver, since they do support their own drivers and kernels. Jesse > -----Original Message----- > From: Brandeburg, Jesse [mailto:[email protected]] > Sent: Thursday, October 21, 2010 2:44 PM > To: Andy Cress > Cc: [email protected] > Subject: Re: [E1000-devel] [PATCH] fix ixgbe preempt bug > > > > On Thu, 21 Oct 2010, Andy Cress wrote: > > > My kernel configuration had CONFIG_PREEMPT=y, and the ixgbe 3.0.12 > > module produced the following error message when it was loaded. > > BUG: using smp_processor_id() in preemptible [00000000] code: > > Below is a patch that works for me to resolve this. Please review it. > > > > this patch is not necessary for any production kernel.org kernels. I'm > not sure what kernel you're running, but maybe they have changed > include/linux/smp.h for some reason to change get_cpu. > > All get_cpu calls defined include a preempt_disable that I can see. > > one other option is that our kcompat code tries to work around kernels > without get_cpu, could that be the issue? > > does your kernel not implement the LINUX_VERSION_CODE macro? > > our code has this which could trigger an override of get_cpu > #if ( LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0) ) > #undef get_cpu > #define get_cpu() smp_processor_id() > #undef put_cpu > #define put_cpu() do { } while(0) > > but that should only be in effect for kernels < 2.6 (aka 2.4.X kernels) > > I suggest putting a #warning in the macro above and see if you hit it. > > > Andy > > > > Signed-off-by: cress <[email protected]> > > --- > > drivers/net/ixgbe/ixgbe_main.c | 22 ++++++++++++++++------ > > 1 files changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ixgbe/ixgbe_main.c > > b/drivers/net/ixgbe/ixgbe_main.c > > index 25135fe..e89f5b6 100644 > > --- a/drivers/net/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ixgbe/ixgbe_main.c > > @@ -677,10 +677,12 @@ static void ixgbe_update_tx_dca(struct > > ixgbe_adapter *adapter, > > static void ixgbe_update_dca(struct ixgbe_q_vector *q_vector) > > { > > struct ixgbe_adapter *adapter = q_vector->adapter; > > - int cpu = get_cpu(); > > + int cpu; > > long r_idx; > > int i; > > > > + preempt_disable(); > > + cpu = smp_processor_id(); /*same as get_cpu()*/ > > if (q_vector->cpu == cpu) > > goto out_no_update; > > > > @@ -701,6 +703,7 @@ static void ixgbe_update_dca(struct ixgbe_q_vector > > *q_vector) > > q_vector->cpu = cpu; > > out_no_update: > > put_cpu(); > > + preempt_enable(); > > } > > > > static void ixgbe_setup_dca(struct ixgbe_adapter *adapter) > > @@ -8015,7 +8018,10 @@ static void ixgbe_netpoll(struct net_device > > *netdev) > > static u16 ixgbe_select_queue(struct net_device *dev, struct sk_buff > > *skb) > > { > > struct ixgbe_adapter *adapter = netdev_priv(dev); > > - int txq = smp_processor_id(); > > + int txq; > > + > > + preempt_disable(); > > + txq = smp_processor_id(); > > > > #ifdef IXGBE_FCOE > > if ((skb->protocol == __constant_htons(ETH_P_FCOE)) || > > @@ -8023,10 +8029,10 @@ static u16 ixgbe_select_queue(struct > net_device > > *dev, struct sk_buff *skb) > > if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED) { > > txq &= > > (adapter->ring_feature[RING_F_FCOE].indices - 1); > > txq += adapter->ring_feature[RING_F_FCOE].mask; > > - return txq; > > + goto out_txq; > > } else if (adapter->flags & IXGBE_FLAG_DCB_ENABLED) { > > txq = adapter->fcoe.up; > > - return txq; > > + goto out_txq; > > } > > } > > > > @@ -8034,7 +8040,7 @@ static u16 ixgbe_select_queue(struct net_device > > *dev, struct sk_buff *skb) > > if (adapter->flags & IXGBE_FLAG_FDIR_HASH_CAPABLE) { > > while (unlikely(txq >= dev->real_num_tx_queues)) > > txq -= dev->real_num_tx_queues; > > - return txq; > > + goto out_txq; > > } > > > > if (adapter->flags & IXGBE_FLAG_DCB_ENABLED) { > > @@ -8043,9 +8049,13 @@ static u16 ixgbe_select_queue(struct net_device > > *dev, struct sk_buff *skb) > > else > > txq = (skb->vlan_tci & > > IXGBE_TX_FLAGS_VLAN_PRIO_MASK) > > >> 13; > > - return txq; > > + goto out_txq; > > } > > + preempt_enable(); > > return skb_tx_hash(dev, skb); > > +out_txq: > > + preempt_enable(); > > + return txq; > > } > > > > #endif /* HAVE_NETDEV_SELECT_QUEUE */ > > > ------------------------------------------------------------------------------ Nokia and AT&T present the 2010 Calling All Innovators-North America contest Create new apps & games for the Nokia N8 for consumers in U.S. and Canada $10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store http://p.sf.net/sfu/nokia-dev2dev _______________________________________________ E1000-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
