| Message ID | 20100415.001446.244372815.davem@davemloft.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Apr 15, 2010 at 3:14 PM, David Miller <davem@davemloft.net> wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 13 Apr 2010 09:14:17 +0200 > >> [PATCH net-next-2.6] net: netif_rx() must disable preemption >> >> Eric Paris reported netif_rx() is calling smp_processor_id() from >> preemptible context, in particular when caller is >> ip_dev_loopback_xmit(). >> >> RPS commit added this smp_processor_id() call, this patch makes sure >> preemption is disabled. rps_get_cpus() wants rcu_read_lock() anyway, we >> can dot it a bit earlier. >> >> Reported-by: Eric Paris <eparis@redhat.com> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > I've applied this with some coding style fixups. > > Thanks! > > -------------------- > net: netif_rx() must disable preemption > > Eric Paris reported netif_rx() is calling smp_processor_id() from > preemptible context, in particular when caller is > ip_dev_loopback_xmit(). > > RPS commit added this smp_processor_id() call, this patch makes sure > preemption is disabled. rps_get_cpus() wants rcu_read_lock() anyway, we > can dot it a bit earlier. > > Reported-by: Eric Paris <eparis@redhat.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > net/core/dev.c | 25 +++++++++++++++---------- > 1 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 876b111..e8041eb 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2206,6 +2206,7 @@ DEFINE_PER_CPU(struct netif_rx_stats, netdev_rx_stat) = { 0, }; > /* > * get_rps_cpu is called from netif_receive_skb and returns the target > * CPU from the RPS map of the receiving queue for a given skb. > + * rcu_read_lock must be held on entry. > */ > static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb) > { > @@ -2217,8 +2218,6 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb) > u8 ip_proto; > u32 addr1, addr2, ports, ihl; > > - rcu_read_lock(); > - > if (skb_rx_queue_recorded(skb)) { > u16 index = skb_get_rx_queue(skb); > if (unlikely(index >= dev->num_rx_queues)) { > @@ -2296,7 +2295,6 @@ got_hash: > } > > done: > - rcu_read_unlock(); > return cpu; > } > > @@ -2392,7 +2390,7 @@ enqueue: > > int netif_rx(struct sk_buff *skb) > { > - int cpu; > + int ret; > > /* if netpoll wants it, pretend we never saw it */ > if (netpoll_rx(skb)) > @@ -2402,14 +2400,21 @@ int netif_rx(struct sk_buff *skb) > net_timestamp(skb); > > #ifdef CONFIG_RPS > - cpu = get_rps_cpu(skb->dev, skb); > - if (cpu < 0) > - cpu = smp_processor_id(); > + { > + int cpu; > + > + rcu_read_lock(); > + cpu = get_rps_cpu(skb->dev, skb); > + if (cpu < 0) > + cpu = smp_processor_id(); > + ret = enqueue_to_backlog(skb, cpu); > + rcu_read_unlock(); > + } > #else > - cpu = smp_processor_id(); > + ret = enqueue_to_backlog(skb, get_cpu()); > + put_cpu(); > #endif > - > - return enqueue_to_backlog(skb, cpu); > + return ret; > } > EXPORT_SYMBOL(netif_rx); > Should netif_rx() be used only when preemption is disabled? If not, netif_rx_ni() should be used instead.?
From: Changli Gao <xiaosuo@gmail.com> Date: Thu, 15 Apr 2010 15:30:44 +0800 > Should netif_rx() be used only when preemption is disabled? If not, > netif_rx_ni() should be used instead.? netif_rx() must be invoked from a hardware or software interrupt, which implies preemption disabled. In netif_rx_ni(), the "ni" means "not interrupt". -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 15, 2010 at 3:37 PM, David Miller <davem@davemloft.net> wrote: > From: Changli Gao <xiaosuo@gmail.com> > Date: Thu, 15 Apr 2010 15:30:44 +0800 > >> Should netif_rx() be used only when preemption is disabled? If not, >> netif_rx_ni() should be used instead.? > > netif_rx() must be invoked from a hardware or software interrupt, > which implies preemption disabled. > > In netif_rx_ni(), the "ni" means "not interrupt". > yea, I know netif_rx_ni()'s meaning. It means that the following changes aren't necessary. #else - cpu = smp_processor_id(); + ret = enqueue_to_backlog(skb, get_cpu()); + put_cpu(); ret = enqueue_to_backlog(skb, smp_processor_id()); should be OK. #endif - - return enqueue_to_backlog(skb, cpu); + return ret; } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Changli Gao <xiaosuo@gmail.com> Date: Thu, 15 Apr 2010 15:47:26 +0800 > On Thu, Apr 15, 2010 at 3:37 PM, David Miller <davem@davemloft.net> wrote: >> From: Changli Gao <xiaosuo@gmail.com> >> Date: Thu, 15 Apr 2010 15:30:44 +0800 >> >>> Should netif_rx() be used only when preemption is disabled? If not, >>> netif_rx_ni() should be used instead.? >> >> netif_rx() must be invoked from a hardware or software interrupt, >> which implies preemption disabled. >> >> In netif_rx_ni(), the "ni" means "not interrupt". >> > > yea, I know netif_rx_ni()'s meaning. It means that the following > changes aren't necessary. > > #else > - cpu = smp_processor_id(); > + ret = enqueue_to_backlog(skb, get_cpu()); > + put_cpu(); > Why? If we are in an interrupt (either soft or hard) then smp_processor_id() is stable, and valid. Changli, I find it very frustrating to communicate with you, you are very terse in your descriptions and analysis and you make many simple errors that would be avoided if you spent more time thinking about things before sending your emails. :-/ Instead of just showing some pseudo patch, state WHY it is needed. Talk about the execution state of environment and what rules or other things are being violated which must be corrected. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/dev.c b/net/core/dev.c index 876b111..e8041eb 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2206,6 +2206,7 @@ DEFINE_PER_CPU(struct netif_rx_stats, netdev_rx_stat) = { 0, }; /* * get_rps_cpu is called from netif_receive_skb and returns the target * CPU from the RPS map of the receiving queue for a given skb. + * rcu_read_lock must be held on entry. */ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb) { @@ -2217,8 +2218,6 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb) u8 ip_proto; u32 addr1, addr2, ports, ihl; - rcu_read_lock(); - if (skb_rx_queue_recorded(skb)) { u16 index = skb_get_rx_queue(skb); if (unlikely(index >= dev->num_rx_queues)) { @@ -2296,7 +2295,6 @@ got_hash: } done: - rcu_read_unlock(); return cpu; } @@ -2392,7 +2390,7 @@ enqueue: int netif_rx(struct sk_buff *skb) { - int cpu; + int ret; /* if netpoll wants it, pretend we never saw it */ if (netpoll_rx(skb)) @@ -2402,14 +2400,21 @@ int netif_rx(struct sk_buff *skb) net_timestamp(skb); #ifdef CONFIG_RPS - cpu = get_rps_cpu(skb->dev, skb); - if (cpu < 0) - cpu = smp_processor_id(); + { + int cpu; + + rcu_read_lock(); + cpu = get_rps_cpu(skb->dev, skb); + if (cpu < 0) + cpu = smp_processor_id(); + ret = enqueue_to_backlog(skb, cpu); + rcu_read_unlock(); + } #else - cpu = smp_processor_id(); + ret = enqueue_to_backlog(skb, get_cpu()); + put_cpu(); #endif - - return enqueue_to_backlog(skb, cpu); + return ret; } EXPORT_SYMBOL(netif_rx);