| Message ID | 4AEE71EC.7040208@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 02 Nov 2009 06:45:16 +0100 > Avoids touching dev refcount in hotpath > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied. -- 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
Eric Dumazet wrote, On 11/02/2009 06:45 AM: > Avoids touching dev refcount in hotpath > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > drivers/net/ifb.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c > index 030913f..69c2566 100644 > --- a/drivers/net/ifb.c > +++ b/drivers/net/ifb.c > @@ -98,13 +98,15 @@ static void ri_tasklet(unsigned long dev) > stats->tx_packets++; > stats->tx_bytes +=skb->len; > > - skb->dev = dev_get_by_index(&init_net, skb->iif); > + rcu_read_lock(); > + skb->dev = dev_get_by_index_rcu(&init_net, skb->iif); > if (!skb->dev) { > + rcu_read_unlock(); > dev_kfree_skb(skb); > stats->tx_dropped++; > break; > } > - dev_put(skb->dev); > + rcu_read_unlock(); I wonder if this rcu_read_unlock() isn't too early here. I know, it functionally fully replaces the old method, but as a whole it looks strange: > rcu_read_lock(); > skb->dev = dev_get_by_index_rcu(&init_net, skb->iif); > if (!skb->dev) { > rcu_read_unlock(); > dev_kfree_skb(skb); > stats->tx_dropped++; > break; > } > rcu_read_unlock(); > skb->iif = _dev->ifindex; > > if (from & AT_EGRESS) { > dp->st_rx_frm_egr++; > dev_queue_xmit(skb); > } else if (from & AT_INGRESS) { > dp->st_rx_frm_ing++; > skb_pull(skb, skb->dev->hard_header_len); So, how is skb->dev protected here, above and below? It seems these rcu read blocks need extending, don't they? Jarek P. > netif_rx(skb); > } else > BUG(); > } -- 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
Jarek Poplawski a écrit : > Eric Dumazet wrote, On 11/02/2009 06:45 AM: > >> Avoids touching dev refcount in hotpath >> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> >> --- >> drivers/net/ifb.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c >> index 030913f..69c2566 100644 >> --- a/drivers/net/ifb.c >> +++ b/drivers/net/ifb.c >> @@ -98,13 +98,15 @@ static void ri_tasklet(unsigned long dev) >> stats->tx_packets++; >> stats->tx_bytes +=skb->len; >> >> - skb->dev = dev_get_by_index(&init_net, skb->iif); >> + rcu_read_lock(); >> + skb->dev = dev_get_by_index_rcu(&init_net, skb->iif); >> if (!skb->dev) { >> + rcu_read_unlock(); >> dev_kfree_skb(skb); >> stats->tx_dropped++; >> break; >> } >> - dev_put(skb->dev); >> + rcu_read_unlock(); > > I wonder if this rcu_read_unlock() isn't too early here. I know, it > functionally fully replaces the old method, but as a whole it looks > strange: > >> rcu_read_lock(); >> skb->dev = dev_get_by_index_rcu(&init_net, skb->iif); >> if (!skb->dev) { >> rcu_read_unlock(); >> dev_kfree_skb(skb); >> stats->tx_dropped++; >> break; >> } >> rcu_read_unlock(); >> skb->iif = _dev->ifindex; >> >> if (from & AT_EGRESS) { >> dp->st_rx_frm_egr++; >> dev_queue_xmit(skb); >> } else if (from & AT_INGRESS) { >> dp->st_rx_frm_ing++; >> skb_pull(skb, skb->dev->hard_header_len); > > > So, how is skb->dev protected here, above and below? It seems these > rcu read blocks need extending, don't they? > Well, this might be true, but we run under tasklet (softirq) with preemption disabled. We might move rcu_read_unlock() some lines down to not rely on this. -- 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 Mon, Nov 02, 2009 at 10:34:04PM +0100, Eric Dumazet wrote: > Jarek Poplawski a écrit : ... > > So, how is skb->dev protected here, above and below? It seems these > > rcu read blocks need extending, don't they? > > > > Well, this might be true, but we run under tasklet (softirq) with preemption disabled. > > We might move rcu_read_unlock() some lines down to not rely on this. I think it's needed now at least for readability. Jarek P. -- 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/drivers/net/ifb.c b/drivers/net/ifb.c index 030913f..69c2566 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -98,13 +98,15 @@ static void ri_tasklet(unsigned long dev) stats->tx_packets++; stats->tx_bytes +=skb->len; - skb->dev = dev_get_by_index(&init_net, skb->iif); + rcu_read_lock(); + skb->dev = dev_get_by_index_rcu(&init_net, skb->iif); if (!skb->dev) { + rcu_read_unlock(); dev_kfree_skb(skb); stats->tx_dropped++; break; } - dev_put(skb->dev); + rcu_read_unlock(); skb->iif = _dev->ifindex; if (from & AT_EGRESS) {
Avoids touching dev refcount in hotpath Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- drivers/net/ifb.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) -- 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