Skip to contentSkip to conversation

net-tun: restructure tun_do_read for better sleep/wakeup efficiency

PATCH In review Xi Wang 3 participants 9 messages May 7, 2014 – May 13, 2014

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

Xi Wang

author
#0 lore raw

PATCH 1/1 net-tun: restructure tun_do_read for better sleep/wakeup efficiency

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

Ran performance tests with the following configuration:

- my packet generator -> tap1 -> br0 -> tap0 -> my packet consumer
- sender pinned to one core and receiver pinned to another core
- sender send small UDP packets (64 bytes total) as fast as it can
- sandy bridge cores
- throughput are receiver side goodput numbers

The results are

baseline: 757k pkts/sec, cpu utilization at 1.54 cpus
changed: 804k pkts/sec, cpu utilization at 1.57 cpus

The performance difference is largely determined by packet rate and inter-cpu communication cost. For example, if the sender and receiver are pinned to different cpu sockets, the results are

baseline: 558k pkts/sec, cpu utilization at 1.71 cpus
changed: 690k pkts/sec, cpu utilization at 1.67 cpus

Co-authored-by Eric Dumazet <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 drivers/net/tun.c | 68 +++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)
drivers/net/tun.c +26 -42
index ee328ba..cb25385 100644
@@ -133,8 +133,7 @@ struct tap_filter {
 struct tun_file {
 	struct sock sk;
 	struct socket socket;
-	struct socket_wq wq;
-	struct tun_struct __rcu *tun;
+	struct tun_struct __rcu *tun ____cacheline_aligned_in_smp;
 	struct net *net;
 	struct fasync_struct *fasync;
 	/* only used for fasnyc */

@@ -498,12 +497,12 @@ static void tun_detach_all(struct net_device *dev)
 	for (i = 0; i < n; i++) {
 		tfile = rtnl_dereference(tun->tfiles[i]);
 		BUG_ON(!tfile);
-		wake_up_all(&tfile->wq.wait);
+		tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 		RCU_INIT_POINTER(tfile->tun, NULL);
 		--tun->numqueues;
 	}
 	list_for_each_entry(tfile, &tun->disabled, next) {
-		wake_up_all(&tfile->wq.wait);
+	tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 		RCU_INIT_POINTER(tfile->tun, NULL);
 	}
 	BUG_ON(tun->numqueues != 0);

@@ -807,8 +806,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Notify and wake up reader process */
 	if (tfile->flags & TUN_FASYNC)
 		kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
-	wake_up_interruptible_poll(&tfile->wq.wait, POLLIN |
-				   POLLRDNORM | POLLRDBAND);
+	tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 
 	rcu_read_unlock();
 	return NETDEV_TX_OK;

@@ -965,7 +963,7 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
 
 	tun_debug(KERN_INFO, tun, "tun_chr_poll\n");
 
-	poll_wait(file, &tfile->wq.wait, wait);
+	poll_wait(file, sk_sleep(sk), wait);
 
 	if (!skb_queue_empty(&sk->sk_receive_queue))
 		mask |= POLLIN | POLLRDNORM;

@@ -1330,46 +1328,21 @@ done:
 static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
 			   const struct iovec *iv, ssize_t len, int noblock)
 {
-	DECLARE_WAITQUEUE(wait, current);
 	struct sk_buff *skb;
 	ssize_t ret = 0;
+	int peeked, err, off = 0;
 
 	tun_debug(KERN_INFO, tun, "tun_do_read\n");
 
-	if (unlikely(!noblock))
-		add_wait_queue(&tfile->wq.wait, &wait);
-	while (len) {
-		if (unlikely(!noblock))
-			current->state = TASK_INTERRUPTIBLE;
-
-		/* Read frames from the queue */
-		if (!(skb = skb_dequeue(&tfile->socket.sk->sk_receive_queue))) {
-			if (noblock) {
-				ret = -EAGAIN;
-				break;
-			}
-			if (signal_pending(current)) {
-				ret = -ERESTARTSYS;
-				break;
-			}
-			if (tun->dev->reg_state != NETREG_REGISTERED) {
-				ret = -EIO;
-				break;
-			}
-
-			/* Nothing to read, let's sleep */
-			schedule();
-			continue;
-		}
+	if (!len)
+		return ret;
 
+	/* Read frames from queue */
+	skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
+				  &peeked, &off, &err);
+	if (skb) {
 		ret = tun_put_user(tun, tfile, skb, iv, len);
 		kfree_skb(skb);
-		break;
-	}
-
-	if (unlikely(!noblock)) {
-		current->state = TASK_RUNNING;
-		remove_wait_queue(&tfile->wq.wait, &wait);
 	}
 
 	return ret;

@@ -2187,20 +2160,28 @@ out:
 static int tun_chr_open(struct inode *inode, struct file * file)
 {
 	struct tun_file *tfile;
+	struct socket_wq *wq;
 
 	DBG1(KERN_INFO, "tunX: tun_chr_open\n");
 
+	wq = kzalloc(sizeof(*wq), GFP_KERNEL);
+	if (!wq)
+		return -ENOMEM;
+
 	tfile = (struct tun_file *)sk_alloc(&init_net, AF_UNSPEC, GFP_KERNEL,
 					    &tun_proto);
-	if (!tfile)
+	if (!tfile) {
+		kfree(wq);
 		return -ENOMEM;
+	}
+
 	RCU_INIT_POINTER(tfile->tun, NULL);
 	tfile->net = get_net(current->nsproxy->net_ns);
 	tfile->flags = 0;
 	tfile->ifindex = 0;
 
-	rcu_assign_pointer(tfile->socket.wq, &tfile->wq);
-	init_waitqueue_head(&tfile->wq.wait);
+	init_waitqueue_head(&wq->wait);
+	RCU_INIT_POINTER(tfile->socket.wq, wq);
 
 	tfile->socket.file = file;
 	tfile->socket.ops = &tun_socket_ops;

@@ -2224,9 +2205,12 @@ static int tun_chr_close(struct inode *inode, struct file *file)
 {
 	struct tun_file *tfile = file->private_data;
 	struct net *net = tfile->net;
+	struct socket_wq *wq;
 
+	wq = rcu_dereference_protected(tfile->socket.wq, 1);
 	tun_detach(tfile, true);
 	put_net(net);
+	kfree_rcu(wq, rcu);
 
 	return 0;
 }
Signature · 1 line
1.9.1.423.g4596e3a
#1 lore raw
Quoted 3 lines · Xi Wang

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 5 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Quoted 14 lines

Ran performance tests with the following configuration:

- my packet generator -> tap1 -> br0 -> tap0 -> my packet consumer
- sender pinned to one core and receiver pinned to another core
- sender send small UDP packets (64 bytes total) as fast as it can
- sandy bridge cores
- throughput are receiver side goodput numbers

The results are

baseline: 757k pkts/sec, cpu utilization at 1.54 cpus
changed: 804k pkts/sec, cpu utilization at 1.57 cpus

Quoted 6 lines

The performance difference is largely determined by packet rate and inter-cpu communication cost. For example, if the sender and receiver are pinned to different cpu sockets, the results are

baseline: 558k pkts/sec, cpu utilization at 1.71 cpus
changed: 690k pkts/sec, cpu utilization at 1.67 cpus

So I believe your consumer is using blocking reads. How about re-test with non blocking reads and re-test to make sure no regression?

Quoted 12 lines
Co-authored-by Eric Dumazet <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 drivers/net/tun.c | 68 +++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)
drivers/net/tun.c +0 -0
index ee328ba..cb25385 100644

drivers/net/tun.c struct tap_filter { · line 136

Quoted 6 lines
drivers/net/tun.c +1 -2
@@ -133,8 +133,7 @@ struct tap_filter {
 struct tun_file {
 	struct sock sk;
 	struct socket socket;
-	struct socket_wq wq;
-	struct tun_struct __rcu *tun;
+	struct tun_struct __rcu *tun ____cacheline_aligned_in_smp;

This seems a optimization which is un-related to the topic. May send as another patch but did you really see improvement for this?

Quoted 70 lines
drivers/net/tun.c +5 -28
 	struct net *net;
 	struct fasync_struct *fasync;
 	/* only used for fasnyc */

@@ -498,12 +497,12 @@ static void tun_detach_all(struct net_device *dev)
 	for (i = 0; i < n; i++) {
 		tfile = rtnl_dereference(tun->tfiles[i]);
 		BUG_ON(!tfile);
-		wake_up_all(&tfile->wq.wait);
+		tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 		RCU_INIT_POINTER(tfile->tun, NULL);
 		--tun->numqueues;
 	}
 	list_for_each_entry(tfile, &tun->disabled, next) {
-		wake_up_all(&tfile->wq.wait);
+	tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 		RCU_INIT_POINTER(tfile->tun, NULL);
 	}
 	BUG_ON(tun->numqueues != 0);

@@ -807,8 +806,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Notify and wake up reader process */
 	if (tfile->flags & TUN_FASYNC)
 		kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
-	wake_up_interruptible_poll(&tfile->wq.wait, POLLIN |
-				   POLLRDNORM | POLLRDBAND);
+	tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 
 	rcu_read_unlock();
 	return NETDEV_TX_OK;

@@ -965,7 +963,7 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
 
 	tun_debug(KERN_INFO, tun, "tun_chr_poll\n");
 
-	poll_wait(file, &tfile->wq.wait, wait);
+	poll_wait(file, sk_sleep(sk), wait);
 
 	if (!skb_queue_empty(&sk->sk_receive_queue))
 		mask |= POLLIN | POLLRDNORM;

@@ -1330,46 +1328,21 @@ done:
 static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
 			   const struct iovec *iv, ssize_t len, int noblock)
 {
-	DECLARE_WAITQUEUE(wait, current);
 	struct sk_buff *skb;
 	ssize_t ret = 0;
+	int peeked, err, off = 0;
 
 	tun_debug(KERN_INFO, tun, "tun_do_read\n");
 
-	if (unlikely(!noblock))
-		add_wait_queue(&tfile->wq.wait, &wait);
-	while (len) {
-		if (unlikely(!noblock))
-			current->state = TASK_INTERRUPTIBLE;
-
-		/* Read frames from the queue */
-		if (!(skb = skb_dequeue(&tfile->socket.sk->sk_receive_queue))) {
-			if (noblock) {
-				ret = -EAGAIN;
-				break;
-			}
-			if (signal_pending(current)) {
-				ret = -ERESTARTSYS;
-				break;
-			}
-			if (tun->dev->reg_state != NETREG_REGISTERED) {
-				ret = -EIO;
-				break;
-			}
-
-			/* Nothing to read, let's sleep */

drivers/net/tun.c done: · line 1343

Quoted 10 lines
drivers/net/tun.c +6 -3
-			schedule();
-			continue;
-		}
+	if (!len)
+		return ret;
 
+	/* Read frames from queue */
+	skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
+				  &peeked, &off, &err);
+	if (skb) {

This changes the userspace ABI a little bit. Originally, userspace can see different error codes and do responds, but here it can only see zero.

Quoted 13 lines
drivers/net/tun.c +0 -6
 		ret = tun_put_user(tun, tfile, skb, iv, len);
 		kfree_skb(skb);
-		break;
-	}
-
-	if (unlikely(!noblock)) {
-		current->state = TASK_RUNNING;
-		remove_wait_queue(&tfile->wq.wait, &wait);
 	}
 
 	return ret;

@@ -2187,20 +2160,28 @@ out:
 static int tun_chr_open(struct inode *inode, struct file * file)

drivers/net/tun.c out: · line 2170

Quoted 10 lines
drivers/net/tun.c +5 -0
 {
 	struct tun_file *tfile;
+	struct socket_wq *wq;
 
 	DBG1(KERN_INFO, "tunX: tun_chr_open\n");
 
+	wq = kzalloc(sizeof(*wq), GFP_KERNEL);
+	if (!wq)
+		return -ENOMEM;
+

Why not just reusing the socket_wq structure inside tun_file structure like what we did in the past?

Quoted 33 lines
drivers/net/tun.c +9 -3
 	tfile = (struct tun_file *)sk_alloc(&init_net, AF_UNSPEC, GFP_KERNEL,
 					    &tun_proto);
-	if (!tfile)
+	if (!tfile) {
+		kfree(wq);
 		return -ENOMEM;
+	}
+
 	RCU_INIT_POINTER(tfile->tun, NULL);
 	tfile->net = get_net(current->nsproxy->net_ns);
 	tfile->flags = 0;
 	tfile->ifindex = 0;
 
-	rcu_assign_pointer(tfile->socket.wq, &tfile->wq);
-	init_waitqueue_head(&tfile->wq.wait);
+	init_waitqueue_head(&wq->wait);
+	RCU_INIT_POINTER(tfile->socket.wq, wq);
 
 	tfile->socket.file = file;
 	tfile->socket.ops = &tun_socket_ops;

@@ -2224,9 +2205,12 @@ static int tun_chr_close(struct inode *inode, struct file *file)
 {
 	struct tun_file *tfile = file->private_data;
 	struct net *net = tfile->net;
+	struct socket_wq *wq;
 
+	wq = rcu_dereference_protected(tfile->socket.wq, 1);
 	tun_detach(tfile, true);
 	put_net(net);
+	kfree_rcu(wq, rcu);
 
 	return 0;
 }
author
#2 lore raw
Quoted 10 lines · Jason Wang
Quoted 3 lines · Xi Wang

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 1 line
Quoted 7 lines
Quoted 4 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Yes this should be a good side effect and I am also interested in trying. Busy polling in user space is not ideal as it doesn't give the lowest latency. Besides differences in interrupt latency etc., there is a bad case for non blocking mode: When a packet arrives right before the polling thread returns to userspace. The control flow has to cross kernel/userspace boundary 3 times before the packet can be processed, while kernel blocking or busy polling only needs 1 boundary crossing.

Quoted 19 lines
Quoted 19 lines

Ran performance tests with the following configuration:

- my packet generator -> tap1 -> br0 -> tap0 -> my packet consumer
- sender pinned to one core and receiver pinned to another core
- sender send small UDP packets (64 bytes total) as fast as it can
- sandy bridge cores
- throughput are receiver side goodput numbers

The results are

baseline: 757k pkts/sec, cpu utilization at 1.54 cpus
changed: 804k pkts/sec, cpu utilization at 1.57 cpus

The performance difference is largely determined by packet rate and inter-cpu communication cost. For example, if the sender and receiver are pinned to different cpu sockets, the results are

Quoted 5 lines
Quoted 2 lines

baseline: 558k pkts/sec, cpu utilization at 1.71 cpus
changed: 690k pkts/sec, cpu utilization at 1.67 cpus

So I believe your consumer is using blocking reads. How about re-test with non blocking reads and re-test to make sure no regression?

I tested non blocking read and found no regression. However the sender is the bottleneck in my case so packet blasting is not a good test for non blocking mode. I switched to RR / ping pong type of traffic through tap. The packet rates for both cases are ~477k and the difference is way below noise.

Quoted 8 lines
Quoted 8 lines
Co-authored-by Eric Dumazet <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 drivers/net/tun.c | 68 +++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)
Quoted 14 lines

drivers/net/tun.c struct tap_filter { · line 136

Quoted 11 lines
drivers/net/tun.c +1 -2
index ee328ba..cb25385 100644
@@ -133,8 +133,7 @@ struct tap_filter {
 struct tun_file {
      struct sock sk;
      struct socket socket;
-     struct socket_wq wq;
-     struct tun_struct __rcu *tun;
+     struct tun_struct __rcu *tun ____cacheline_aligned_in_smp;

This seems a optimization which is un-related to the topic. May send as another patch but did you really see improvement for this?

There is an ~1% difference (not as reliable as other data since the difference is small). This is not a major performance contributor.

Quoted 77 lines
Quoted 77 lines
drivers/net/tun.c +7 -31
      struct net *net;
      struct fasync_struct *fasync;
      /* only used for fasnyc */

@@ -498,12 +497,12 @@ static void tun_detach_all(struct net_device *dev)
      for (i = 0; i < n; i++) {
              tfile = rtnl_dereference(tun->tfiles[i]);
              BUG_ON(!tfile);
-             wake_up_all(&tfile->wq.wait);
+             tfile->socket.sk->sk_data_ready(tfile->socket.sk);
              RCU_INIT_POINTER(tfile->tun, NULL);
              --tun->numqueues;
      }
      list_for_each_entry(tfile, &tun->disabled, next) {
-             wake_up_all(&tfile->wq.wait);
+     tfile->socket.sk->sk_data_ready(tfile->socket.sk);
              RCU_INIT_POINTER(tfile->tun, NULL);
      }
      BUG_ON(tun->numqueues != 0);

@@ -807,8 +806,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
      /* Notify and wake up reader process */
      if (tfile->flags & TUN_FASYNC)
              kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
-     wake_up_interruptible_poll(&tfile->wq.wait, POLLIN |
-                                POLLRDNORM | POLLRDBAND);
+     tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 
      rcu_read_unlock();
      return NETDEV_TX_OK;

@@ -965,7 +963,7 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
 
      tun_debug(KERN_INFO, tun, "tun_chr_poll\n");
 
-     poll_wait(file, &tfile->wq.wait, wait);
+     poll_wait(file, sk_sleep(sk), wait);
 
      if (!skb_queue_empty(&sk->sk_receive_queue))
              mask |= POLLIN | POLLRDNORM;

@@ -1330,46 +1328,21 @@ done:
 static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
                         const struct iovec *iv, ssize_t len, int noblock)
 {
-     DECLARE_WAITQUEUE(wait, current);
      struct sk_buff *skb;
      ssize_t ret = 0;
+     int peeked, err, off = 0;
 
      tun_debug(KERN_INFO, tun, "tun_do_read\n");
 
-     if (unlikely(!noblock))
-             add_wait_queue(&tfile->wq.wait, &wait);
-     while (len) {
-             if (unlikely(!noblock))
-                     current->state = TASK_INTERRUPTIBLE;
-
-             /* Read frames from the queue */
-             if (!(skb = skb_dequeue(&tfile->socket.sk->sk_receive_queue))) {
-                     if (noblock) {
-                             ret = -EAGAIN;
-                             break;
-                     }
-                     if (signal_pending(current)) {
-                             ret = -ERESTARTSYS;
-                             break;
-                     }
-                     if (tun->dev->reg_state != NETREG_REGISTERED) {
-                             ret = -EIO;
-                             break;
-                     }
-
-                     /* Nothing to read, let's sleep */
-                     schedule();
-                     continue;
-             }
+     if (!len)
+             return ret;
Quoted 7 lines

drivers/net/tun.c done: · line 1343

Quoted 4 lines
drivers/net/tun.c +4 -0
+     /* Read frames from queue */
+     skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
+                               &peeked, &off, &err);
+     if (skb) {

This changes the userspace ABI a little bit. Originally, userspace can see different error codes and do responds, but here it can only see zero.

Thanks for catching this! Seems forwarding the &err parameter of __skb_recv_datagram
should get the most of the error code compatibility back? I'll check related code.

Quoted 20 lines
Quoted 20 lines
drivers/net/tun.c +1 -6
              ret = tun_put_user(tun, tfile, skb, iv, len);
              kfree_skb(skb);
-             break;
-     }
-
-     if (unlikely(!noblock)) {
-             current->state = TASK_RUNNING;
-             remove_wait_queue(&tfile->wq.wait, &wait);
      }
 
      return ret;

@@ -2187,20 +2160,28 @@ out:
 static int tun_chr_open(struct inode *inode, struct file * file)
 {
      struct tun_file *tfile;
+     struct socket_wq *wq;
 
      DBG1(KERN_INFO, "tunX: tun_chr_open\n");
Quoted 7 lines

drivers/net/tun.c out: · line 2170

Quoted 4 lines
drivers/net/tun.c +4 -0
+     wq = kzalloc(sizeof(*wq), GFP_KERNEL);
+     if (!wq)
+             return -ENOMEM;
+

Why not just reusing the socket_wq structure inside tun_file structure like what we did in the past?

There is no strong reason for going either way. Changing to dynamic allocation is based on: Less chance of cacheline contention and syncing the code pattern with core stack.

-Xi

#3 lore raw
Quoted 15 lines · Xi Wang
Quoted 7 lines · Jason Wang
Quoted 3 lines · Xi Wang

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 6 lines
Quoted 4 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Quoted 7 lines

Yes this should be a good side effect and I am also interested in trying. Busy polling in user space is not ideal as it doesn't give the lowest latency. Besides differences in interrupt latency etc., there is a bad case for non blocking mode: When a packet arrives right before the polling thread returns to userspace. The control flow has to cross kernel/userspace boundary 3 times before the packet can be processed, while kernel blocking or busy polling only needs 1 boundary crossing.

So if we want to implement this, we need a feature bit to turn it on. Then vhost may benefit from this.

Quoted 138 lines
Quoted 15 lines
Quoted 15 lines

Ran performance tests with the following configuration:

- my packet generator -> tap1 -> br0 -> tap0 -> my packet consumer
- sender pinned to one core and receiver pinned to another core
- sender send small UDP packets (64 bytes total) as fast as it can
- sandy bridge cores
- throughput are receiver side goodput numbers

The results are

baseline: 757k pkts/sec, cpu utilization at 1.54 cpus
changed: 804k pkts/sec, cpu utilization at 1.57 cpus

Quoted 8 lines
Quoted 6 lines

The performance difference is largely determined by packet rate and inter-cpu communication cost. For example, if the sender and receiver are pinned to different cpu sockets, the results are

baseline: 558k pkts/sec, cpu utilization at 1.71 cpus
changed: 690k pkts/sec, cpu utilization at 1.67 cpus

So I believe your consumer is using blocking reads. How about re-test with non blocking reads and re-test to make sure no regression?

I tested non blocking read and found no regression. However the sender is the bottleneck in my case so packet blasting is not a good test for non blocking mode. I switched to RR / ping pong type of traffic through tap. The packet rates for both cases are ~477k and the difference is way below noise.

Quoted 6 lines
Quoted 6 lines
Co-authored-by Eric Dumazet <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 drivers/net/tun.c | 68 +++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)
Quoted 13 lines

drivers/net/tun.c struct tap_filter { · line 136

Quoted 11 lines
drivers/net/tun.c +1 -2
index ee328ba..cb25385 100644
@@ -133,8 +133,7 @@ struct tap_filter {
 struct tun_file {
      struct sock sk;
      struct socket socket;
-     struct socket_wq wq;
-     struct tun_struct __rcu *tun;
+     struct tun_struct __rcu *tun ____cacheline_aligned_in_smp;

This seems a optimization which is un-related to the topic. May send as another patch but did you really see improvement for this?

There is an ~1% difference (not as reliable as other data since the difference is small). This is not a major performance contributor.

Quoted 76 lines
Quoted 76 lines
drivers/net/tun.c +7 -31
      struct net *net;
      struct fasync_struct *fasync;
      /* only used for fasnyc */

@@ -498,12 +497,12 @@ static void tun_detach_all(struct net_device *dev)
      for (i = 0; i < n; i++) {
              tfile = rtnl_dereference(tun->tfiles[i]);
              BUG_ON(!tfile);
-             wake_up_all(&tfile->wq.wait);
+             tfile->socket.sk->sk_data_ready(tfile->socket.sk);
              RCU_INIT_POINTER(tfile->tun, NULL);
              --tun->numqueues;
      }
      list_for_each_entry(tfile, &tun->disabled, next) {
-             wake_up_all(&tfile->wq.wait);
+     tfile->socket.sk->sk_data_ready(tfile->socket.sk);
              RCU_INIT_POINTER(tfile->tun, NULL);
      }
      BUG_ON(tun->numqueues != 0);

@@ -807,8 +806,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
      /* Notify and wake up reader process */
      if (tfile->flags & TUN_FASYNC)
              kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
-     wake_up_interruptible_poll(&tfile->wq.wait, POLLIN |
-                                POLLRDNORM | POLLRDBAND);
+     tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 
      rcu_read_unlock();
      return NETDEV_TX_OK;

@@ -965,7 +963,7 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
 
      tun_debug(KERN_INFO, tun, "tun_chr_poll\n");
 
-     poll_wait(file, &tfile->wq.wait, wait);
+     poll_wait(file, sk_sleep(sk), wait);
 
      if (!skb_queue_empty(&sk->sk_receive_queue))
              mask |= POLLIN | POLLRDNORM;

@@ -1330,46 +1328,21 @@ done:
 static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
                         const struct iovec *iv, ssize_t len, int noblock)
 {
-     DECLARE_WAITQUEUE(wait, current);
      struct sk_buff *skb;
      ssize_t ret = 0;
+     int peeked, err, off = 0;
 
      tun_debug(KERN_INFO, tun, "tun_do_read\n");
 
-     if (unlikely(!noblock))
-             add_wait_queue(&tfile->wq.wait, &wait);
-     while (len) {
-             if (unlikely(!noblock))
-                     current->state = TASK_INTERRUPTIBLE;
-
-             /* Read frames from the queue */
-             if (!(skb = skb_dequeue(&tfile->socket.sk->sk_receive_queue))) {
-                     if (noblock) {
-                             ret = -EAGAIN;
-                             break;
-                     }
-                     if (signal_pending(current)) {
-                             ret = -ERESTARTSYS;
-                             break;
-                     }
-                     if (tun->dev->reg_state != NETREG_REGISTERED) {
-                             ret = -EIO;
-                             break;
-                     }
-
-                     /* Nothing to read, let's sleep */
-                     schedule();
-                     continue;
-             }
+     if (!len)
+             return ret;
Quoted 6 lines

drivers/net/tun.c done: · line 1343

Quoted 4 lines
drivers/net/tun.c +4 -0
+     /* Read frames from queue */
+     skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
+                               &peeked, &off, &err);
+     if (skb) {

This changes the userspace ABI a little bit. Originally, userspace can see different error codes and do responds, but here it can only see zero.

Quoted 3 lines

Thanks for catching this! Seems forwarding the &err parameter of __skb_recv_datagram
should get the most of the error code compatibility back?

Seems not, -ERESTARTSYS and EIO were missed.

Quoted 30 lines

I'll check
related code.

Quoted 17 lines
Quoted 17 lines
drivers/net/tun.c +1 -6
              ret = tun_put_user(tun, tfile, skb, iv, len);
              kfree_skb(skb);
-             break;
-     }
-
-     if (unlikely(!noblock)) {
-             current->state = TASK_RUNNING;
-             remove_wait_queue(&tfile->wq.wait, &wait);
      }
 
      return ret;

@@ -2187,20 +2160,28 @@ out:
 static int tun_chr_open(struct inode *inode, struct file * file)
 {
      struct tun_file *tfile;
+     struct socket_wq *wq;
Quoted 8 lines

drivers/net/tun.c out: · line 2170

Quoted 6 lines
drivers/net/tun.c +4 -0
      DBG1(KERN_INFO, "tunX: tun_chr_open\n");
 
+     wq = kzalloc(sizeof(*wq), GFP_KERNEL);
+     if (!wq)
+             return -ENOMEM;
+

Why not just reusing the socket_wq structure inside tun_file structure like what we did in the past?

Quoted 3 lines

There is no strong reason for going either way. Changing to dynamic allocation is based on: Less chance of cacheline contention and syncing the code pattern with core stack.

It's seems another possible optimization un-related to the topic, better send with another patch. But I suspect how much it will help for the performance.

Checking the other socket implementation such as af unix socket, the socket_wq structure were also embedded in the parent socket structure.

Quoted 7 lines

-Xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/net/tun.c +4 -0
+     /* Read frames from queue */
+     skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
+                               &peeked, &off, &err);
+     if (skb) {
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

This changes the userspace ABI a little bit. Originally, userspace can see different error codes and do responds, but here it can only see zero.

Thanks for catching this! Seems forwarding the &err parameter of __skb_recv_datagram
should get the most of the error code compatibility back?

Seems not, -ERESTARTSYS and EIO were missed.

-ERESTARTSYS would be returned through sock_intr_errno -> wait_for_more_packets -> __skb_recv_datagram., -EIO would still be missing.

-Xi

#5 lore raw
In reply to Jason Wang
Quoted 24 lines · Jason Wang
Quoted 15 lines · Xi Wang
Quoted 7 lines · Jason Wang
Quoted 3 lines · Xi Wang

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 6 lines
Quoted 4 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Quoted 7 lines

Yes this should be a good side effect and I am also interested in trying. Busy polling in user space is not ideal as it doesn't give the lowest latency. Besides differences in interrupt latency etc., there is a bad case for non blocking mode: When a packet arrives right before the polling thread returns to userspace. The control flow has to cross kernel/userspace boundary 3 times before the packet can be processed, while kernel blocking or busy polling only needs 1 boundary crossing.

Quoted 2 lines

So if we want to implement this, we need a feature bit to turn it on. Then vhost may benefit from this.

IFF_TUN_POLL_BUSY_LOOP ? I'm not sure it has to be a flag. Maybe an ioctl is better, if userspace misconfigures this it is only hurting itself, right? Maybe add a module parameter to control polling timeout, or reuse low_latency_poll.

Quoted 195 lines
Quoted 138 lines
Quoted 15 lines
Quoted 15 lines

Ran performance tests with the following configuration:

- my packet generator -> tap1 -> br0 -> tap0 -> my packet consumer
- sender pinned to one core and receiver pinned to another core
- sender send small UDP packets (64 bytes total) as fast as it can
- sandy bridge cores
- throughput are receiver side goodput numbers

The results are

baseline: 757k pkts/sec, cpu utilization at 1.54 cpus
changed: 804k pkts/sec, cpu utilization at 1.57 cpus

Quoted 8 lines
Quoted 6 lines

The performance difference is largely determined by packet rate and inter-cpu communication cost. For example, if the sender and receiver are pinned to different cpu sockets, the results are

baseline: 558k pkts/sec, cpu utilization at 1.71 cpus
changed: 690k pkts/sec, cpu utilization at 1.67 cpus

So I believe your consumer is using blocking reads. How about re-test with non blocking reads and re-test to make sure no regression?

I tested non blocking read and found no regression. However the sender is the bottleneck in my case so packet blasting is not a good test for non blocking mode. I switched to RR / ping pong type of traffic through tap. The packet rates for both cases are ~477k and the difference is way below noise.

Quoted 6 lines
Quoted 6 lines
Co-authored-by Eric Dumazet <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 drivers/net/tun.c | 68 +++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)
Quoted 13 lines

drivers/net/tun.c struct tap_filter { · line 136

Quoted 11 lines
drivers/net/tun.c +1 -2
index ee328ba..cb25385 100644
@@ -133,8 +133,7 @@ struct tap_filter {
 struct tun_file {
      struct sock sk;
      struct socket socket;
-     struct socket_wq wq;
-     struct tun_struct __rcu *tun;
+     struct tun_struct __rcu *tun ____cacheline_aligned_in_smp;

This seems a optimization which is un-related to the topic. May send as another patch but did you really see improvement for this?

There is an ~1% difference (not as reliable as other data since the difference is small). This is not a major performance contributor.

Quoted 76 lines
Quoted 76 lines
drivers/net/tun.c +7 -31
      struct net *net;
      struct fasync_struct *fasync;
      /* only used for fasnyc */

@@ -498,12 +497,12 @@ static void tun_detach_all(struct net_device *dev)
      for (i = 0; i < n; i++) {
              tfile = rtnl_dereference(tun->tfiles[i]);
              BUG_ON(!tfile);
-             wake_up_all(&tfile->wq.wait);
+             tfile->socket.sk->sk_data_ready(tfile->socket.sk);
              RCU_INIT_POINTER(tfile->tun, NULL);
              --tun->numqueues;
      }
      list_for_each_entry(tfile, &tun->disabled, next) {
-             wake_up_all(&tfile->wq.wait);
+     tfile->socket.sk->sk_data_ready(tfile->socket.sk);
              RCU_INIT_POINTER(tfile->tun, NULL);
      }
      BUG_ON(tun->numqueues != 0);

@@ -807,8 +806,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
      /* Notify and wake up reader process */
      if (tfile->flags & TUN_FASYNC)
              kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
-     wake_up_interruptible_poll(&tfile->wq.wait, POLLIN |
-                                POLLRDNORM | POLLRDBAND);
+     tfile->socket.sk->sk_data_ready(tfile->socket.sk);
 
      rcu_read_unlock();
      return NETDEV_TX_OK;

@@ -965,7 +963,7 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
 
      tun_debug(KERN_INFO, tun, "tun_chr_poll\n");
 
-     poll_wait(file, &tfile->wq.wait, wait);
+     poll_wait(file, sk_sleep(sk), wait);
 
      if (!skb_queue_empty(&sk->sk_receive_queue))
              mask |= POLLIN | POLLRDNORM;

@@ -1330,46 +1328,21 @@ done:
 static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
                         const struct iovec *iv, ssize_t len, int noblock)
 {
-     DECLARE_WAITQUEUE(wait, current);
      struct sk_buff *skb;
      ssize_t ret = 0;
+     int peeked, err, off = 0;
 
      tun_debug(KERN_INFO, tun, "tun_do_read\n");
 
-     if (unlikely(!noblock))
-             add_wait_queue(&tfile->wq.wait, &wait);
-     while (len) {
-             if (unlikely(!noblock))
-                     current->state = TASK_INTERRUPTIBLE;
-
-             /* Read frames from the queue */
-             if (!(skb = skb_dequeue(&tfile->socket.sk->sk_receive_queue))) {
-                     if (noblock) {
-                             ret = -EAGAIN;
-                             break;
-                     }
-                     if (signal_pending(current)) {
-                             ret = -ERESTARTSYS;
-                             break;
-                     }
-                     if (tun->dev->reg_state != NETREG_REGISTERED) {
-                             ret = -EIO;
-                             break;
-                     }
-
-                     /* Nothing to read, let's sleep */
-                     schedule();
-                     continue;
-             }
+     if (!len)
+             return ret;
Quoted 6 lines

drivers/net/tun.c done: · line 1343

Quoted 4 lines
drivers/net/tun.c +4 -0
+     /* Read frames from queue */
+     skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
+                               &peeked, &off, &err);
+     if (skb) {

This changes the userspace ABI a little bit. Originally, userspace can see different error codes and do responds, but here it can only see zero.

Quoted 3 lines

Thanks for catching this! Seems forwarding the &err parameter of __skb_recv_datagram
should get the most of the error code compatibility back?

Seems not, -ERESTARTSYS and EIO were missed.

Quoted 30 lines

I'll check
related code.

Quoted 17 lines
Quoted 17 lines
drivers/net/tun.c +1 -6
              ret = tun_put_user(tun, tfile, skb, iv, len);
              kfree_skb(skb);
-             break;
-     }
-
-     if (unlikely(!noblock)) {
-             current->state = TASK_RUNNING;
-             remove_wait_queue(&tfile->wq.wait, &wait);
      }
 
      return ret;

@@ -2187,20 +2160,28 @@ out:
 static int tun_chr_open(struct inode *inode, struct file * file)
 {
      struct tun_file *tfile;
+     struct socket_wq *wq;
Quoted 8 lines

drivers/net/tun.c out: · line 2170

Quoted 6 lines
drivers/net/tun.c +4 -0
      DBG1(KERN_INFO, "tunX: tun_chr_open\n");
 
+     wq = kzalloc(sizeof(*wq), GFP_KERNEL);
+     if (!wq)
+             return -ENOMEM;
+

Why not just reusing the socket_wq structure inside tun_file structure like what we did in the past?

Quoted 3 lines

There is no strong reason for going either way. Changing to dynamic allocation is based on: Less chance of cacheline contention and syncing the code pattern with core stack.

It's seems another possible optimization un-related to the topic, better send with another patch. But I suspect how much it will help for the performance.

Checking the other socket implementation such as af unix socket, the socket_wq structure were also embedded in the parent socket structure.

Quoted 7 lines

-Xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw
Quoted 25 lines · Michael S. Tsirkin
Quoted 24 lines · Jason Wang
Quoted 24 lines · Xi Wang
Quoted 22 lines
Quoted 22 lines · Jason Wang
Quoted 7 lines
Quoted 7 lines · Xi Wang
Quoted 3 lines
Quoted 3 lines

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Yes this should be a good side effect and I am also interested in trying. Busy polling in user space is not ideal as it doesn't give the lowest latency. Besides differences in interrupt latency etc., there is a bad case for non blocking mode: When a packet arrives right before the polling thread returns to userspace. The control flow has to cross kernel/userspace boundary 3 times before the packet can be processed, while kernel blocking or busy polling only needs 1 boundary crossing.

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

So if we want to implement this, we need a feature bit to turn it on. Then vhost may benefit from this.

IFF_TUN_POLL_BUSY_LOOP ? I'm not sure it has to be a flag. Maybe an ioctl is better, if userspace misconfigures this it is only hurting itself, right?

Flag has the same effect. But adding new ioctls means userspace needs to be modified. This is different with current rx busy polling for tcp/udp socket which is transparent to userspace application.

Quoted 3 lines

Maybe add a module parameter to control polling timeout, or reuse low_latency_poll.

If we don't need a global parameter, we can just implement it without generic helper like __skb_recv_datagram().

#7 lore raw
Quoted 28 lines · Jason Wang
Quoted 27 lines · Michael S. Tsirkin
Quoted 26 lines · Jason Wang
Quoted 26 lines · Xi Wang
Quoted 15 lines
Quoted 15 lines · Jason Wang
Quoted 13 lines
Quoted 13 lines · Xi Wang
Quoted 3 lines
Quoted 3 lines

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 4 lines
Quoted 4 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Quoted 7 lines
Quoted 7 lines

Yes this should be a good side effect and I am also interested in trying. Busy polling in user space is not ideal as it doesn't give the lowest latency. Besides differences in interrupt latency etc., there is a bad case for non blocking mode: When a packet arrives right before the polling thread returns to userspace. The control flow has to cross kernel/userspace boundary 3 times before the packet can be processed, while kernel blocking or busy polling only needs 1 boundary crossing.

So if we want to implement this, we need a feature bit to turn it on. Then vhost may benefit from this.

Quoted 7 lines
Quoted 3 lines

IFF_TUN_POLL_BUSY_LOOP ? I'm not sure it has to be a flag. Maybe an ioctl is better, if userspace misconfigures this it is only hurting itself, right?

Flag has the same effect. But adding new ioctls means userspace needs to be modified. This is different with current rx busy polling for tcp/udp socket which is transparent to userspace application.

OTOH risk is much lower though.

Quoted 6 lines
Quoted 3 lines

Maybe add a module parameter to control polling timeout, or reuse low_latency_poll.

If we don't need a global parameter, we can just implement it without generic helper like __skb_recv_datagram().

not sure I get the meaning here.

#8 lore raw
Quoted 33 lines · Michael S. Tsirkin
Quoted 24 lines · Jason Wang
Quoted 23 lines · Michael S. Tsirkin
Quoted 22 lines · Jason Wang
Quoted 22 lines · Xi Wang
Quoted 21 lines
Quoted 21 lines · Jason Wang
Quoted 7 lines
Quoted 7 lines · Xi Wang
Quoted 3 lines
Quoted 3 lines

tun_do_read always adds current thread to wait queue, even if a packet is ready to read. This is inefficient because both sleeper and waker want to acquire the wait queue spin lock when packet rate is high.

After commit 61a5ff15ebdab87887861a6b128b108404e4706d, this will only help for blocking read. Looks like for performance critical userspaces, they will use non blocking reads.

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines

We restructure the read function and use common kernel networking routines to handle receive, sleep and wakeup. With the change available packets are checked first before the reading thread is added to the wait queue.

This is interesting, since it may help if we want to add rx busy loop for tun. (In fact I worked a similar patch like this).

Yes this should be a good side effect and I am also interested in trying. Busy polling in user space is not ideal as it doesn't give the lowest latency. Besides differences in interrupt latency etc., there is a bad case for non blocking mode: When a packet arrives right before the polling thread returns to userspace. The control flow has to cross kernel/userspace boundary 3 times before the packet can be processed, while kernel blocking or busy polling only needs 1 boundary crossing.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

So if we want to implement this, we need a feature bit to turn it on. Then vhost may benefit from this.

IFF_TUN_POLL_BUSY_LOOP ? I'm not sure it has to be a flag. Maybe an ioctl is better, if userspace misconfigures this it is only hurting itself, right?

Flag has the same effect. But adding new ioctls means userspace needs to be modified. This is different with current rx busy polling for tcp/udp socket which is transparent to userspace application.

Quoted 9 lines

OTOH risk is much lower though.

Quoted 5 lines
Quoted 3 lines

Maybe add a module parameter to control polling timeout, or reuse low_latency_poll.

If we don't need a global parameter, we can just implement it without generic helper like __skb_recv_datagram().

not sure I get the meaning here.

I mean current __skb_recv_datagram() does not accept a polling timeout as parameter. So we need either extend it or just call sk_can_busy_loop()/sk_busy_loop() in tun_do_read().