tun: export underlying socket

vhost net module wants to do copy to/from user from a kernel thread, which needs use_mm. Export it to modules.

Acked-by Andrea Arcangeli <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 mm/mmu_context.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

mm/mmu_context.c +3 -0

index ded9081..0777654 100644
@@ -5,6 +5,7 @@
 
 #include <linux/mm.h>
 #include <linux/mmu_context.h>
+#include <linux/module.h>
 #include <linux/sched.h>
 
 #include <asm/mmu_context.h>

@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
 	if (active_mm != mm)
 		mmdrop(active_mm);
 }
+EXPORT_SYMBOL_GPL(use_mm);
 
 /*
  * unuse_mm

@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
 	enter_lazy_tlb(mm, tsk);
 	task_unlock(tsk);
 }
+EXPORT_SYMBOL_GPL(unuse_mm);

vhost net module wants to do copy to/from user from a kernel thread, which needs use_mm. Export it to modules.

Acked-by Andrea Arcangeli <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 mm/mmu_context.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

mm/mmu_context.c +2 -0

index ded9081..0777654 100644
@@ -5,6 +5,7 @@
 
 #include <linux/mm.h>
 #include <linux/mmu_context.h>
+#include <linux/module.h>
 #include <linux/sched.h>
 
 #include <asm/mmu_context.h>

@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
 	if (active_mm != mm)
 		mmdrop(active_mm);
 }
+EXPORT_SYMBOL_GPL(use_mm);
 
 /*
  * unuse_mm

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Kind Regards,
-Greg

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Kind Regards,
-Greg

Quoted 48 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

Quoted 48 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

Quoted 27 lines · Gregory Haskins

Quoted 16 lines · Gregory Haskins

Quoted 5 lines · Eric Dumazet

Michael S. Tsirkin a écrit :

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Quoted 8 lines

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Kind Regards,
-Greg

Here it is:

Quoted 2 lines

http://lkml.org/lkml/2009/8/12/173

Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use. People wanting to use RCU do a grep on kernel sources to find how to correctly use RCU.

Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Quoted 27 lines · Gregory Haskins

Quoted 16 lines · Gregory Haskins

Quoted 5 lines · Eric Dumazet

Michael S. Tsirkin a écrit :

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Quoted 8 lines

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Kind Regards,
-Greg

Here it is:

Quoted 2 lines

http://lkml.org/lkml/2009/8/12/173

Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use. People wanting to use RCU do a grep on kernel sources to find how to correctly use RCU.

Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Quoted 49 lines · Gregory Haskins

Quoted 48 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 8 lines

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use. People wanting to use RCU do a grep on kernel sources to find how to correctly use RCU.

Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Some automatic tools might barf later.

For example, we could add a debugging facility to check that rcu_dereference() is used in an appropriate context, ie conflict with existing mutex_lock() debugging facility.

Quoted 49 lines · Gregory Haskins

Quoted 48 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 8 lines

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use. People wanting to use RCU do a grep on kernel sources to find how to correctly use RCU.

Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Some automatic tools might barf later.

For example, we could add a debugging facility to check that rcu_dereference() is used in an appropriate context, ie conflict with existing mutex_lock() debugging facility.

Paul, you acked this previously. Should I add you acked-by line so people calm down? If you would rather I replace rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this. Or maybe patch Documentation to explain this RCU usage?

Paul, you acked this previously. Should I add you acked-by line so people calm down? If you would rather I replace rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this. Or maybe patch Documentation to explain this RCU usage?

Quoted 49 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

Quoted 49 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

drivers/vhost/net.c new file +16 -0

+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;

drivers/vhost/net.c new file +16 -0

+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;

Quoted 50 lines · Gregory Haskins

Quoted 49 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 7 lines

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

What was happening in that case was that the rcu_dereference() was being used in a workqueue item. The role of rcu_read_lock() was taken on be the start of execution of the workqueue item, of rcu_read_unlock() by the end of execution of the workqueue item, and of synchronize_rcu() by flush_workqueue(). This does work, at least assuming that flush_workqueue() operates as advertised, which it appears to at first glance.

The above code looks somewhat different, however -- I don't see handle_tx() being executed in the context of a work queue. Instead it appears to be in an interrupt handler.
So what is the story? Using synchronize_irq() or some such?

Thanx, Paul

Quoted 50 lines · Gregory Haskins

Quoted 49 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 7 lines

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

What was happening in that case was that the rcu_dereference() was being used in a workqueue item. The role of rcu_read_lock() was taken on be the start of execution of the workqueue item, of rcu_read_unlock() by the end of execution of the workqueue item, and of synchronize_rcu() by flush_workqueue(). This does work, at least assuming that flush_workqueue() operates as advertised, which it appears to at first glance.

The above code looks somewhat different, however -- I don't see handle_tx() being executed in the context of a work queue. Instead it appears to be in an interrupt handler.
So what is the story? Using synchronize_irq() or some such?

Thanx, Paul

Quoted 6 lines · Michael S. Tsirkin

Paul, you acked this previously. Should I add you acked-by line so people calm down? If you would rather I replace rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this. Or maybe patch Documentation to explain this RCU usage?

So you believe I am over-reacting to this dubious use of RCU ?

RCU documentation is already very complex, we dont need to add yet another subtle use, and makes it less readable.

It seems you use 'RCU api' in drivers/vhost/net.c as convenient macros :

#define rcu_dereference(p)     ({ \
                                typeof(p) _________p1 = ACCESS_ONCE(p); \
                                smp_read_barrier_depends(); \
                                (_________p1); \
                                })

#define rcu_assign_pointer(p, v) \
        ({ \
                if (!__builtin_constant_p(v) || \
                    ((v) != NULL)) \
                        smp_wmb(); \
                (p) = (v); \
        })

There are plenty regular uses of smp_wmb() in kernel, not related to Read Copy Update, there is nothing wrong to use barriers with appropriate comments.

(And you already use mb(), wmb(), rmb(), smp_wmb() in your patch)

BTW there is at least one locking bug in vhost_net_set_features()

Apparently, mutex_unlock() doesnt trigger a fault if mutex is not locked by current thread... even with DEBUG_MUTEXES / DEBUG_LOCK_ALLOC

static void vhost_net_set_features(struct vhost_net *n, u64 features)

{
       size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
               sizeof(struct virtio_net_hdr) : 0;
       int i;
<<!>>  mutex_unlock(&n->dev.mutex);
       n->dev.acked_features = features;
       smp_wmb();
       for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
               mutex_lock(&n->vqs[i].mutex);
               n->vqs[i].hdr_size = hdr_size;
               mutex_unlock(&n->vqs[i].mutex);
       }
       mutex_unlock(&n->dev.mutex);
       vhost_net_flush(n);
}

Quoted 6 lines · Michael S. Tsirkin

Paul, you acked this previously. Should I add you acked-by line so people calm down? If you would rather I replace rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this. Or maybe patch Documentation to explain this RCU usage?

So you believe I am over-reacting to this dubious use of RCU ?

RCU documentation is already very complex, we dont need to add yet another subtle use, and makes it less readable.

It seems you use 'RCU api' in drivers/vhost/net.c as convenient macros :

#define rcu_dereference(p)     ({ \
                                typeof(p) _________p1 = ACCESS_ONCE(p); \
                                smp_read_barrier_depends(); \
                                (_________p1); \
                                })

#define rcu_assign_pointer(p, v) \
        ({ \
                if (!__builtin_constant_p(v) || \
                    ((v) != NULL)) \
                        smp_wmb(); \
                (p) = (v); \
        })

There are plenty regular uses of smp_wmb() in kernel, not related to Read Copy Update, there is nothing wrong to use barriers with appropriate comments.

(And you already use mb(), wmb(), rmb(), smp_wmb() in your patch)

BTW there is at least one locking bug in vhost_net_set_features()

Apparently, mutex_unlock() doesnt trigger a fault if mutex is not locked by current thread... even with DEBUG_MUTEXES / DEBUG_LOCK_ALLOC

static void vhost_net_set_features(struct vhost_net *n, u64 features)

{
       size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
               sizeof(struct virtio_net_hdr) : 0;
       int i;
<<!>>  mutex_unlock(&n->dev.mutex);
       n->dev.acked_features = features;
       smp_wmb();
       for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
               mutex_lock(&n->vqs[i].mutex);
               n->vqs[i].hdr_size = hdr_size;
               mutex_unlock(&n->vqs[i].mutex);
       }
       mutex_unlock(&n->dev.mutex);
       vhost_net_flush(n);
}

"Michael S. Tsirkin" <[email protected]> writes:

Haven't really read the whole thing, just noticed something at a glance.

Quoted 22 lines

drivers/vhost/net.c new file +22 -0

+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)
+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);

drivers/vhost/net.c line 119

Quoted 4 lines

drivers/vhost/net.c new file +4 -0

+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);

I haven't gone over all this code in detail, but that isolated reference count use looks suspicious. What prevents the mm from going away before you increment, if it's not the current one?

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

"Michael S. Tsirkin" <[email protected]> writes:

Haven't really read the whole thing, just noticed something at a glance.

Quoted 22 lines

drivers/vhost/net.c new file +22 -0

+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)
+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);

drivers/vhost/net.c line 119

Quoted 4 lines

drivers/vhost/net.c new file +4 -0

+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);

I haven't gone over all this code in detail, but that isolated reference count use looks suspicious. What prevents the mm from going away before you increment, if it's not the current one?

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Fine?

Fine?

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

(as in do you hold a reference to it?)

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

(as in do you hold a reference to it?)

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 2 lines

(as in do you hold a reference to it?)

By design I think I always have a reference to mm before I use it.

This works like this:
ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue ioctl RESET_OWNER - stops virtqueues, drops the reference to mm file close - stops virtqueues, if we still have it then drops mm

This is why I think I can call use_mm/unuse_mm while virtqueue is running, safely.
Makes sense?

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 2 lines

(as in do you hold a reference to it?)

By design I think I always have a reference to mm before I use it.

This works like this:
ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue ioctl RESET_OWNER - stops virtqueues, drops the reference to mm file close - stops virtqueues, if we still have it then drops mm

This is why I think I can call use_mm/unuse_mm while virtqueue is running, safely.
Makes sense?

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

No the reference to the device, which owns the mm for you.

Quoted 11 lines

Quoted 2 lines

(as in do you hold a reference to it?)

By design I think I always have a reference to mm before I use it.

This works like this:
ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue ioctl RESET_OWNER - stops virtqueues, drops the reference to mm file close - stops virtqueues, if we still have it then drops mm

Quoted 3 lines

This is why I think I can call use_mm/unuse_mm while virtqueue is running, safely.
Makes sense?

Do you protect against another thread doing RESET_OWNER in parallel while RESET_OWNER runs?

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

No the reference to the device, which owns the mm for you.

Quoted 11 lines

Quoted 2 lines

(as in do you hold a reference to it?)

By design I think I always have a reference to mm before I use it.

This works like this:
ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue ioctl RESET_OWNER - stops virtqueues, drops the reference to mm file close - stops virtqueues, if we still have it then drops mm

Quoted 3 lines

This is why I think I can call use_mm/unuse_mm while virtqueue is running, safely.
Makes sense?

Do you protect against another thread doing RESET_OWNER in parallel while RESET_OWNER runs?

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Assuming it's okay with davem, I think it makes sense to merge this patch through Rusty's tree because vhost is the first user of the new interface. Posted here for completeness.

Assuming it's okay with davem, I think it makes sense to merge this patch through Rusty's tree because vhost is the first user of the new interface. Posted here for completeness.

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 1 line

No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed when file is closed. So I think the fs code handles the reference counting for me: it won't call file cleanup callback while some userspace process has the file open. Right?

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 1 line

No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed when file is closed. So I think the fs code handles the reference counting for me: it won't call file cleanup callback while some userspace process has the file open. Right?

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 7 lines

Quoted 1 line

No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed when file is closed. So I think the fs code handles the reference counting for me: it won't call file cleanup callback while some userspace process has the file open. Right?

Yes.

But the semantics when someone inherits such a fd through exec or through file descriptor passing would be surely "interesting" You would still do IO on the old VM.

I guess it would be a good way to confuse memory accounting schemes or administrators @)
It would be all saner if this was all a single atomic step.

-Andi

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 7 lines

Quoted 1 line

No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed when file is closed. So I think the fs code handles the reference counting for me: it won't call file cleanup callback while some userspace process has the file open. Right?

Yes.

But the semantics when someone inherits such a fd through exec or through file descriptor passing would be surely "interesting" You would still do IO on the old VM.

I guess it would be a good way to confuse memory accounting schemes or administrators @)
It would be all saner if this was all a single atomic step.

-Andi

Quoted 24 lines · Andi Kleen

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 7 lines

Quoted 1 line

No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed when file is closed. So I think the fs code handles the reference counting for me: it won't call file cleanup callback while some userspace process has the file open. Right?

Yes.

But the semantics when someone inherits such a fd through exec or through file descriptor passing would be surely "interesting" You would still do IO on the old VM.

Quoted 5 lines

I guess it would be a good way to confuse memory accounting schemes or administrators @)
It would be all saner if this was all a single atomic step.

-Andi

I have this atomic actually. A child process will first thing do SET_OWNER: this is required before any other operation.

SET_OWNER atomically (under mutex) does two things:
- check that there is no other owner
- get mm and set current process as owner

I hope this addresses your concern?

Quoted 24 lines · Andi Kleen

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Andi Kleen

Quoted 1 line

Fine?

I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

Quoted 7 lines

Quoted 1 line

No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed when file is closed. So I think the fs code handles the reference counting for me: it won't call file cleanup callback while some userspace process has the file open. Right?

Yes.

But the semantics when someone inherits such a fd through exec or through file descriptor passing would be surely "interesting" You would still do IO on the old VM.

Quoted 5 lines

I guess it would be a good way to confuse memory accounting schemes or administrators @)
It would be all saner if this was all a single atomic step.

-Andi

I have this atomic actually. A child process will first thing do SET_OWNER: this is required before any other operation.

SET_OWNER atomically (under mutex) does two things:
- check that there is no other owner
- get mm and set current process as owner

I hope this addresses your concern?

Quoted 72 lines · Paul E. McKenney

Quoted 50 lines · Gregory Haskins

Quoted 49 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 7 lines

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

What was happening in that case was that the rcu_dereference() was being used in a workqueue item. The role of rcu_read_lock() was taken on be the start of execution of the workqueue item, of rcu_read_unlock() by the end of execution of the workqueue item, and of synchronize_rcu() by flush_workqueue(). This does work, at least assuming that flush_workqueue() operates as advertised, which it appears to at first glance.

The above code looks somewhat different, however -- I don't see handle_tx() being executed in the context of a work queue. Instead it appears to be in an interrupt handler.
So what is the story? Using synchronize_irq() or some such?

Quoted 1 line

Thanx, Paul

No, there has been no change (I won't be able to use a mutex in an interrupt handler, will I?). handle_tx is still called in the context of a work queue: either from handle_tx_kick or from handle_tx_net which are work queue items.

Can you ack this usage please?

Quoted 72 lines · Paul E. McKenney

Quoted 50 lines · Gregory Haskins

Quoted 49 lines · Gregory Haskins

Quoted 31 lines · Eric Dumazet

Quoted 17 lines · Michael S. Tsirkin

drivers/vhost/net.c new file +17 -0

+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+	unsigned head, out, in, s;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_iov = vq->iov,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err, wmem;
+	size_t hdr_size;
+	struct socket *sock = rcu_dereference(vq->private_data);
+	if (!sock)

drivers/vhost/net.c line 122

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		return;
+
+	wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+	if (wmem >= sock->sk->sk_sndbuf)
+		return;
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
+

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect that your use of RCU is not correct.

Quoted 7 lines

1) rcu_dereference() should be done inside a read_rcu_lock() section, and we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to find the thread.

Quoted 7 lines

Quoted 3 lines

Kind Regards,
-Greg

Here it is:

http://lkml.org/lkml/2009/8/12/173

What was happening in that case was that the rcu_dereference() was being used in a workqueue item. The role of rcu_read_lock() was taken on be the start of execution of the workqueue item, of rcu_read_unlock() by the end of execution of the workqueue item, and of synchronize_rcu() by flush_workqueue(). This does work, at least assuming that flush_workqueue() operates as advertised, which it appears to at first glance.

The above code looks somewhat different, however -- I don't see handle_tx() being executed in the context of a work queue. Instead it appears to be in an interrupt handler.
So what is the story? Using synchronize_irq() or some such?

Quoted 1 line

Thanx, Paul

No, there has been no change (I won't be able to use a mutex in an interrupt handler, will I?). handle_tx is still called in the context of a work queue: either from handle_tx_kick or from handle_tx_net which are work queue items.

Can you ack this usage please?

(Sorry, but, as always, I could not resist!)

(Sorry, but, as always, I could not resist!)

(Sorry, but, as always, I could not resist!)

Thanx, Paul

(Sorry, but, as always, I could not resist!)

Thanx, Paul

Quoted 3 lines · Paul E. McKenney

(Sorry, but, as always, I could not resist!)

Thanx, Paul

Thanks Paul!
Jonathan: are you reading this?
Another one for your quotes of the week collection :)

Quoted 3 lines · Paul E. McKenney

(Sorry, but, as always, I could not resist!)

Thanx, Paul

Thanks Paul!
Jonathan: are you reading this?
Another one for your quotes of the week collection :)

Quoted 1 line · Michael S. Tsirkin

Can you ack this usage please?

I thought I had done so in my paragraph above, but if you would like something a bit more formal...

Quoted 1 line · Michael S. Tsirkin

Can you ack this usage please?

I thought I had done so in my paragraph above, but if you would like something a bit more formal...

Quoted 5 lines · Paul E. McKenney

Quoted 1 line · Michael S. Tsirkin

Can you ack this usage please?

I thought I had done so in my paragraph above, but if you would like something a bit more formal...

<snip verbose super-ack with qualifications>

That's great guys. And yes, this is a kind of read-copy-update. And no, there's nothing wrong with it.

But it's still nasty to use half an API. If it were a few places I would have open-coded it with a comment, or wrapped it. As it is, I don't think that would be a win.

Quoted 5 lines · Paul E. McKenney

Quoted 1 line · Michael S. Tsirkin

Can you ack this usage please?

I thought I had done so in my paragraph above, but if you would like something a bit more formal...

<snip verbose super-ack with qualifications>

That's great guys. And yes, this is a kind of read-copy-update. And no, there's nothing wrong with it.

But it's still nasty to use half an API. If it were a few places I would have open-coded it with a comment, or wrapped it. As it is, I don't think that would be a win.

Quoted 3 lines · Rusty Russell

But it's still nasty to use half an API. If it were a few places I would have open-coded it with a comment, or wrapped it. As it is, I don't think that would be a win.

So would it help to have a rcu_read_lock_workqueue() and rcu_read_unlock_workqueue() that checked nesting and whether they were actually running in the context of a workqueue item? Or did you have something else in mind? Or am I misjudging the level of sarcasm in your reply? ;-)

Quoted 3 lines · Rusty Russell

But it's still nasty to use half an API. If it were a few places I would have open-coded it with a comment, or wrapped it. As it is, I don't think that would be a win.

So would it help to have a rcu_read_lock_workqueue() and rcu_read_unlock_workqueue() that checked nesting and whether they were actually running in the context of a workqueue item? Or did you have something else in mind? Or am I misjudging the level of sarcasm in your reply? ;-)

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Rusty Russell

But it's still nasty to use half an API. If it were a few places I would have open-coded it with a comment, or wrapped it. As it is, I don't think that would be a win.

Quoted 5 lines

So would it help to have a rcu_read_lock_workqueue() and rcu_read_unlock_workqueue() that checked nesting and whether they were actually running in the context of a workqueue item? Or did you have something else in mind? Or am I misjudging the level of sarcasm in your reply? ;-)

You read correctly. If we get a second user, creating an API makes sense.

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Rusty Russell

But it's still nasty to use half an API. If it were a few places I would have open-coded it with a comment, or wrapped it. As it is, I don't think that would be a win.

Quoted 5 lines

So would it help to have a rcu_read_lock_workqueue() and rcu_read_unlock_workqueue() that checked nesting and whether they were actually running in the context of a workqueue item? Or did you have something else in mind? Or am I misjudging the level of sarcasm in your reply? ;-)

You read correctly. If we get a second user, creating an API makes sense.