Skip to contentSkip to conversation

tun: use netif_receive_skb instead of netif_rx_ni

PATCH In review Qin Chuanyu 5 participants 12 messages Feb 11, 2014 – Feb 16, 2014

Subsystems (4)
Networking Network drivers KVM TUN/TAP driver
lore
Share & tools
lore

Qin Chuanyu

author
#0 lore raw

PATCH 1/1 tun: use netif_receive_skb instead of netif_rx_ni

we could xmit directly instead of going through softirq to gain throughput and lantency improved. test model: VM-Host-Host just do transmit. with vhost thread and nic
interrupt bind cpu1. netperf do throuhput test and qperf do lantency test.
Host OS: suse11sp3, Guest OS: suse11sp3

latency result(us):
packet_len 64 256 512 1460

old(UDP)   44  47  48   66
new(UDP)   38  41  42   66
old(TCP)   52  55  70  117
new(TCP)   45  48  61  114

throughput result(Gbit/s):

packet_len   64   512   1024   1460
old(UDP)   0.42  2.02   3.75   4.68
new(UDP)   0.45  2.14   3.77   5.06

TCP due to the latency, client couldn't send packet big enough to get benefit from TSO of nic, so the result show it will send more packet per sencond but get lower throughput.

Eric mentioned that it would has problem with cgroup, but the patch had been sent by Herbert Xu.
patch_id f845172531fb7410c7fb7780b1a6e51ee6df7d52

Signed-off-by Chuanyu Qin <[email protected]>
---
 drivers/net/tun.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	skb_probe_transport_header(skb, 0);
 
 	rxhash = skb_get_hash(skb);
-	netif_rx_ni(skb);
+	rcu_read_lock_bh();
+	netif_receive_skb(skb);
+	rcu_read_unlock_bh();
 
 	tun->dev->stats.rx_packets++;
 	tun->dev->stats.rx_bytes += len;
Signature · 1 line
1.7.3.1.msysgit.0
#1 lore raw
Quoted 47 lines · Qin Chuanyu

we could xmit directly instead of going through softirq to gain throughput and lantency improved.
test model: VM-Host-Host just do transmit. with vhost thread and nic interrupt bind cpu1. netperf do throuhput test and qperf do lantency test. Host OS: suse11sp3, Guest OS: suse11sp3

latency result(us):
packet_len 64 256 512 1460

old(UDP)   44  47  48   66
new(UDP)   38  41  42   66
old(TCP)   52  55  70  117
new(TCP)   45  48  61  114

throughput result(Gbit/s):

packet_len   64   512   1024   1460
old(UDP)   0.42  2.02   3.75   4.68
new(UDP)   0.45  2.14   3.77   5.06

TCP due to the latency, client couldn't send packet big enough to get benefit from TSO of nic, so the result show it will send more packet per sencond but get lower throughput.

Eric mentioned that it would has problem with cgroup, but the patch had been sent by Herbert Xu.
patch_id f845172531fb7410c7fb7780b1a6e51ee6df7d52

Signed-off-by Chuanyu Qin <[email protected]>
---
  drivers/net/tun.c |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct 
 *tun, struct tun_file *tfile,
  	skb_probe_transport_header(skb, 0);
 
  	rxhash = skb_get_hash(skb);
-	netif_rx_ni(skb);
+	rcu_read_lock_bh();
+	netif_receive_skb(skb);
+	rcu_read_unlock_bh();
 
  	tun->dev->stats.rx_packets++;
Quoted 1 line
  	tun->dev->stats.rx_bytes += len;

I already said this patch is not good :

rcu_read_lock_bh() makes no sense here.

What is really needed is local_bh_disable();

Herbert patch ( http://patchwork.ozlabs.org/patch/52963/ ) had a much cleaner form.

Just use it, CC him, credit him, please ?

#2 lore raw
Quoted 52 lines · Eric Dumazet
Quoted 47 lines · Qin Chuanyu

we could xmit directly instead of going through softirq to gain throughput and lantency improved.
test model: VM-Host-Host just do transmit. with vhost thread and nic interrupt bind cpu1. netperf do throuhput test and qperf do lantency test. Host OS: suse11sp3, Guest OS: suse11sp3

latency result(us):
packet_len 64 256 512 1460

old(UDP)   44  47  48   66
new(UDP)   38  41  42   66
old(TCP)   52  55  70  117
new(TCP)   45  48  61  114

throughput result(Gbit/s):

packet_len   64   512   1024   1460
old(UDP)   0.42  2.02   3.75   4.68
new(UDP)   0.45  2.14   3.77   5.06

TCP due to the latency, client couldn't send packet big enough to get benefit from TSO of nic, so the result show it will send more packet per sencond but get lower throughput.

Eric mentioned that it would has problem with cgroup, but the patch had been sent by Herbert Xu.
patch_id f845172531fb7410c7fb7780b1a6e51ee6df7d52

Signed-off-by Chuanyu Qin <[email protected]>
---
  drivers/net/tun.c |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct 
 *tun, struct tun_file *tfile,
  	skb_probe_transport_header(skb, 0);
 
  	rxhash = skb_get_hash(skb);
-	netif_rx_ni(skb);
+	rcu_read_lock_bh();
+	netif_receive_skb(skb);
+	rcu_read_unlock_bh();
 
  	tun->dev->stats.rx_packets++;
Quoted 1 line
  	tun->dev->stats.rx_bytes += len;

I already said this patch is not good :

Quoted 9 lines

rcu_read_lock_bh() makes no sense here.

What is really needed is local_bh_disable();

Herbert patch ( http://patchwork.ozlabs.org/patch/52963/ ) had a much cleaner form.

Just use it, CC him, credit him, please ?

But not before making sure (testing) that patch does not break the cgroups classifier please.

Signature · 1 line
MST
author
#3 lore raw
In reply to Eric Dumazet
Quoted 19 lines · Eric Dumazet
Quoted 15 lines
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct
 *tun, struct tun_file *tfile,
   	skb_probe_transport_header(skb, 0);
 
   	rxhash = skb_get_hash(skb);
-	netif_rx_ni(skb);
+	rcu_read_lock_bh();
+	netif_receive_skb(skb);
+	rcu_read_unlock_bh();
 
   	tun->dev->stats.rx_packets++;
Quoted 1 line
   	tun->dev->stats.rx_bytes += len;

I already said this patch is not good :

Quoted 9 lines

rcu_read_lock_bh() makes no sense here.

What is really needed is local_bh_disable();

Herbert patch ( http://patchwork.ozlabs.org/patch/52963/ ) had a much cleaner form.

Just use it, CC him, credit him, please ?

To: Herbert Xu
I saw that you had delivered a patch to resolve the problem of cgroup. patch num is f845172531fb7410c7fb7780b1a6e51ee6df7d52

so would you deliver your patch for tun again? I had test it and found it work well.

To: Eric
I approve your idea that rcu_read_lock make no sense. but I couldn't
understand why dev_queue_xmit use it.
What is the difference between vhost thread using tap to xmit skb and another thread use dev_queue_xmit to xmit skb？ It is really my honor if you could explain it for me.
Thanks.

#5 lore raw
In reply to Qin Chuanyu
Quoted 24 lines · Qin Chuanyu

we could xmit directly instead of going through softirq to gain throughput and lantency improved.
test model: VM-Host-Host just do transmit. with vhost thread and nic interrupt bind cpu1. netperf do throuhput test and qperf do lantency test.
Host OS: suse11sp3, Guest OS: suse11sp3

latency result(us):
packet_len 64 256 512 1460

old(UDP)   44  47  48   66
new(UDP)   38  41  42   66
old(TCP)   52  55  70  117
new(TCP)   45  48  61  114

throughput result(Gbit/s):

packet_len   64   512   1024   1460
old(UDP)   0.42  2.02   3.75   4.68
new(UDP)   0.45  2.14   3.77   5.06

TCP due to the latency, client couldn't send packet big enough to get benefit from TSO of nic, so the result show it will send more packet per sencond but get lower throughput.

Quoted 6 lines

Eric mentioned that it would has problem with cgroup, but the patch had been sent by Herbert Xu.
patch_id f845172531fb7410c7fb7780b1a6e51ee6df7d52

Signed-off-by Chuanyu Qin <[email protected]>

---

A question: without NAPI weight, could this starve other net devices?

Quoted 19 lines
 drivers/net/tun.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct
 *tun, struct tun_file *tfile,
     skb_probe_transport_header(skb, 0);
 
     rxhash = skb_get_hash(skb);
-    netif_rx_ni(skb);
+    rcu_read_lock_bh();
+    netif_receive_skb(skb);
+    rcu_read_unlock_bh();
 
     tun->dev->stats.rx_packets++;

     tun->dev->stats.rx_bytes += len;
#6 lore raw
Quoted 1 line · Jason Wang

A question: without NAPI weight, could this starve other net devices?

Not really, as net devices are serviced by softirq handler.

#7 lore raw
Quoted 6 lines · Eric Dumazet
Quoted 1 line · Jason Wang

A question: without NAPI weight, could this starve other net devices?

Not really, as net devices are serviced by softirq handler.

Yes, then the issue is tun could be starved by other net devices.

#8 lore raw
Quoted 9 lines · Jason Wang
Quoted 6 lines · Eric Dumazet
Quoted 1 line · Jason Wang

A question: without NAPI weight, could this starve other net devices?

Not really, as net devices are serviced by softirq handler.

Yes, then the issue is tun could be starved by other net devices.

How this patch changes anything to this 'problem' ?

netif_rx_ni() can only be called if your process is not preempted by other high prio tasks/softirqs.

If this process is scheduled on a cpu, then disabling bh to process _one_ packet wont fundamentally change dynamic of the system.

author
#10 lore raw
In reply to Jason Wang
Quoted 1 line · Jason Wang

A question: without NAPI weight, could this starve other net devices?

tap xmit skb use thread context，the poll func of physical nic driver could be called in softirq context without change.

I had test it by binding vhost thread and physic nic interrupt on the same vcpu, use netperf xmit udp, test model is VM1-Host1-Host2.

if only VM1 xmit skb, the top show as below :
Cpu1 :0.0%us, 95.0%sy, 0.0%ni, 0.0%id, 0.0%wa, 0.0%hi, 5.0%si, 0.0%st

then use host2 xmit skb to VM1, the top show as below :
Cpu1 :0.0%us, 41.0%sy, 0.0%ni, 0.0%id, 0.0%wa, 0.0%hi, 59.0%si, 0.0%st

so I think there is no problem with this change.

Quoted 23 lines
Quoted 18 lines
  drivers/net/tun.c |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct
 *tun, struct tun_file *tfile,
      skb_probe_transport_header(skb, 0);
 
      rxhash = skb_get_hash(skb);
-    netif_rx_ni(skb);
+    rcu_read_lock_bh();
+    netif_receive_skb(skb);
+    rcu_read_unlock_bh();
 
      tun->dev->stats.rx_packets++;
Quoted 1 line
      tun->dev->stats.rx_bytes += len;

.

#9 lore raw
In reply to Eric Dumazet
Quoted 8 lines · Eric Dumazet
Quoted 7 lines · Jason Wang
Quoted 6 lines · Eric Dumazet
Quoted 1 line · Jason Wang

A question: without NAPI weight, could this starve other net devices?

Not really, as net devices are serviced by softirq handler.

Quoted 9 lines
Quoted 1 line

Yes, then the issue is tun could be starved by other net devices.

How this patch changes anything to this 'problem' ?

netif_rx_ni() can only be called if your process is not preempted by other high prio tasks/softirqs.

If this process is scheduled on a cpu, then disabling bh to process _one_ packet wont fundamentally change dynamic of the system.

After looking at the code for a while, I agree it won't be a great change.

Thanks

#11 lore raw
In reply to Qin Chuanyu
Quoted 9 lines · Qin Chuanyu
Quoted 1 line · Jason Wang

A question: without NAPI weight, could this starve other net devices?

tap xmit skb use thread context，the poll func of physical nic driver could be called in softirq context without change.

I had test it by binding vhost thread and physic nic interrupt on the same vcpu, use netperf xmit udp, test model is VM1-Host1-Host2.

Quoted 7 lines

if only VM1 xmit skb, the top show as below :
Cpu1 :0.0%us, 95.0%sy, 0.0%ni, 0.0%id, 0.0%wa, 0.0%hi, 5.0%si, 0.0%st

then use host2 xmit skb to VM1, the top show as below :
Cpu1 :0.0%us, 41.0%sy, 0.0%ni, 0.0%id, 0.0%wa, 0.0%hi, 59.0%si, 0.0%st

so I think there is no problem with this change.

Yes, I realize it was ok after Eric's comment.

Thanks

#4 lore raw
In reply to Qin Chuanyu
Quoted 27 lines · Qin Chuanyu
Quoted 26 lines · Eric Dumazet
Quoted 15 lines
drivers/net/tun.c +3 -1
index 44c4db8..90b4e58 100644
@@ -1184,7 +1184,9 @@ static ssize_t tun_get_user(struct tun_struct
 *tun, struct tun_file *tfile,
  	skb_probe_transport_header(skb, 0);
 
  	rxhash = skb_get_hash(skb);
-	netif_rx_ni(skb);
+	rcu_read_lock_bh();
+	netif_receive_skb(skb);
+	rcu_read_unlock_bh();
 
  	tun->dev->stats.rx_packets++;
Quoted 1 line
  	tun->dev->stats.rx_bytes += len;

I already said this patch is not good :

rcu_read_lock_bh() makes no sense here.

What is really needed is local_bh_disable();

Herbert patch ( http://patchwork.ozlabs.org/patch/52963/ ) had a much cleaner form.

Quoted 8 lines
Quoted 2 lines

Just use it, CC him, credit him, please ?

To: Herbert Xu
I saw that you had delivered a patch to resolve the problem of cgroup. patch num is f845172531fb7410c7fb7780b1a6e51ee6df7d52

so would you deliver your patch for tun again? I had test it and found it work well.

Feel free to resubmit my patch either as is or with your modifications as I no longer need it for my original purposes.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt