ifb: RCU locking avoids touching dev refcount
Subsystems (2)
PATCH 1/1 ifb: RCU locking avoids touching dev refcount
Avoids touching dev refcount in hotpath
--- drivers/net/ifb.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/net/ifb.c +4 -2
@@ -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 ) {
From: Eric Dumazet <[email protected]>
Date: Mon, 02 Nov 2009 06:45:16 +0100
Quoted 3 lines
Avoids touching dev refcount in hotpath
Signed-off-by Eric Dumazet <[email protected]>
Applied.
Eric Dumazet wrote, On 11/02/2009 06:45 AM:
Quoted 16 lines
Avoids touching dev refcount in hotpath
Signed-off-by Eric Dumazet <[email protected]>--- drivers/net/ifb.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)drivers/net/ifb.c +0
-1@@ -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);
drivers/net/ifb.c static void ri_tasklet(unsigned long dev) · line 109
Quoted 10 lines
drivers/net/ifb.c +4
-1+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:
Quoted 9 lines
drivers/net/ifb.c +0
-0rcu_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();
Quoted 8 lines
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.
Quoted 4 lines
netif_rx(skb);
} else
BUG();
}Quoted 52 lines · Jarek Poplawski
Eric Dumazet wrote, On 11/02/2009 06:45 AM:
Quoted 16 lines
Avoids touching dev refcount in hotpath
Signed-off-by Eric Dumazet <[email protected]>--- drivers/net/ifb.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)drivers/net/ifb.c +0
-1@@ -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);drivers/net/ifb.c static void ri_tasklet(unsigned long dev) · line 109
Quoted 10 lines
drivers/net/ifb.c +4
-1+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:Quoted 9 lines
drivers/net/ifb.c +0
-0rcu_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();Quoted 8 lines
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);
Quoted 3 lines
So, how is
skb->devprotected 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.
Quoted 1 line · Eric Dumazet
Jarek Poplawski a écrit :
...
Quoted 7 lines
Quoted 3 lines
So, how is
skb->devprotected 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.