Re: [PATCH V2 5/6] vhost_net: poll vhost queue after marking DMA is done
On 02/12/2014 03:38 PM, Qin Chuanyu wrote: On 2013/8/30 12:29, Jason Wang wrote: We used to poll vhost queue before making DMA is done, this is racy if vhost thread were waked up before marking DMA is done which can result the signal to be missed. Fix this by always poll the vhost thread before DMA is done. Signed-off-by: Jason Wang jasow...@redhat.com --- drivers/vhost/net.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index ff60c2a..d09c17c 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -308,6 +308,11 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) struct vhost_virtqueue *vq = ubufs-vq; int cnt = atomic_read(ubufs-kref.refcount); +/* set len to mark this desc buffers done DMA */ +vq-heads[ubuf-desc].len = success ? +VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; +vhost_net_ubuf_put(ubufs); + /* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1 @@ -318,10 +323,6 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) */ if (cnt = 2 || !(cnt % 16)) vhost_poll_queue(vq-poll); -/* set len to mark this desc buffers done DMA */ -vq-heads[ubuf-desc].len = success ? -VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; -vhost_net_ubuf_put(ubufs); } /* Expects to be always run from workqueue - which acts as with this change, vq would lose protection that provided by ubufs-kref. if another thread is waiting at vhost_net_ubuf_put_and_wait called by vhost_net_release, then after vhost_net_ubuf_put, vq would been free by vhost_net_release soon, vhost_poll_queue(vq-poll) may cause NULL pointer Exception. Good catch. another question is that vhost_zerocopy_callback is called by kfree_skb, it may called in different thread context. vhost_poll_queue is called decided by ubufs-kref.refcount, this may cause there isn't any thread call vhost_poll_queue, but at least one is needed. and this cause network break. We could repeat it by using 8 netperf thread in guest to xmit tcp to its host. I think if using atomic_read to decide while do vhost_poll_queue or not, at least a spink_lock is needed. Then you need another ref count to protect that spinlock? Care to send patches? Thanks -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 5/6] vhost_net: poll vhost queue after marking DMA is done
On Wed, Feb 12, 2014 at 03:38:18PM +0800, Qin Chuanyu wrote: another question is that vhost_zerocopy_callback is called by kfree_skb, it may called in different thread context. vhost_poll_queue is called decided by ubufs-kref.refcount, this may cause there isn't any thread call vhost_poll_queue, but at least one is needed. and this cause network break. We could repeat it by using 8 netperf thread in guest to xmit tcp to its host. Thanks a lot for the report, will send the patch soon. I think if using atomic_read to decide while do vhost_poll_queue or not, at least a spink_lock is needed. No, nothing so drastic. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 5/6] vhost_net: poll vhost queue after marking DMA is done
On 2013/8/30 12:29, Jason Wang wrote: We used to poll vhost queue before making DMA is done, this is racy if vhost thread were waked up before marking DMA is done which can result the signal to be missed. Fix this by always poll the vhost thread before DMA is done. Signed-off-by: Jason Wang jasow...@redhat.com --- drivers/vhost/net.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index ff60c2a..d09c17c 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -308,6 +308,11 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) struct vhost_virtqueue *vq = ubufs-vq; int cnt = atomic_read(ubufs-kref.refcount); + /* set len to mark this desc buffers done DMA */ + vq-heads[ubuf-desc].len = success ? + VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; + vhost_net_ubuf_put(ubufs); + /* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1 @@ -318,10 +323,6 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) */ if (cnt = 2 || !(cnt % 16)) vhost_poll_queue(vq-poll); - /* set len to mark this desc buffers done DMA */ - vq-heads[ubuf-desc].len = success ? - VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; - vhost_net_ubuf_put(ubufs); } /* Expects to be always run from workqueue - which acts as with this change, vq would lose protection that provided by ubufs-kref. if another thread is waiting at vhost_net_ubuf_put_and_wait called by vhost_net_release, then after vhost_net_ubuf_put, vq would been free by vhost_net_release soon, vhost_poll_queue(vq-poll) may cause NULL pointer Exception. another question is that vhost_zerocopy_callback is called by kfree_skb, it may called in different thread context. vhost_poll_queue is called decided by ubufs-kref.refcount, this may cause there isn't any thread call vhost_poll_queue, but at least one is needed. and this cause network break. We could repeat it by using 8 netperf thread in guest to xmit tcp to its host. I think if using atomic_read to decide while do vhost_poll_queue or not, at least a spink_lock is needed. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 5/6] vhost_net: poll vhost queue after marking DMA is done
On 08/31/2013 12:44 AM, Ben Hutchings wrote: On Fri, 2013-08-30 at 12:29 +0800, Jason Wang wrote: We used to poll vhost queue before making DMA is done, this is racy if vhost thread were waked up before marking DMA is done which can result the signal to be missed. Fix this by always poll the vhost thread before DMA is done. Does this bug only exist in net-next or is it older? Should the fix go to net and stable branches? This should go for the stable branches too (3.4 above). Thanks for the checking. Ben. Signed-off-by: Jason Wang jasow...@redhat.com --- drivers/vhost/net.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index ff60c2a..d09c17c 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -308,6 +308,11 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) struct vhost_virtqueue *vq = ubufs-vq; int cnt = atomic_read(ubufs-kref.refcount); +/* set len to mark this desc buffers done DMA */ +vq-heads[ubuf-desc].len = success ? +VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; +vhost_net_ubuf_put(ubufs); + /* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1 @@ -318,10 +323,6 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) */ if (cnt = 2 || !(cnt % 16)) vhost_poll_queue(vq-poll); -/* set len to mark this desc buffers done DMA */ -vq-heads[ubuf-desc].len = success ? -VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; -vhost_net_ubuf_put(ubufs); } /* Expects to be always run from workqueue - which acts as ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 5/6] vhost_net: poll vhost queue after marking DMA is done
On Fri, 2013-08-30 at 12:29 +0800, Jason Wang wrote: We used to poll vhost queue before making DMA is done, this is racy if vhost thread were waked up before marking DMA is done which can result the signal to be missed. Fix this by always poll the vhost thread before DMA is done. Does this bug only exist in net-next or is it older? Should the fix go to net and stable branches? Ben. Signed-off-by: Jason Wang jasow...@redhat.com --- drivers/vhost/net.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index ff60c2a..d09c17c 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -308,6 +308,11 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) struct vhost_virtqueue *vq = ubufs-vq; int cnt = atomic_read(ubufs-kref.refcount); + /* set len to mark this desc buffers done DMA */ + vq-heads[ubuf-desc].len = success ? + VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; + vhost_net_ubuf_put(ubufs); + /* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1 @@ -318,10 +323,6 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) */ if (cnt = 2 || !(cnt % 16)) vhost_poll_queue(vq-poll); - /* set len to mark this desc buffers done DMA */ - vq-heads[ubuf-desc].len = success ? - VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; - vhost_net_ubuf_put(ubufs); } /* Expects to be always run from workqueue - which acts as -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V2 5/6] vhost_net: poll vhost queue after marking DMA is done
We used to poll vhost queue before making DMA is done, this is racy if vhost thread were waked up before marking DMA is done which can result the signal to be missed. Fix this by always poll the vhost thread before DMA is done. Signed-off-by: Jason Wang jasow...@redhat.com --- drivers/vhost/net.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index ff60c2a..d09c17c 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -308,6 +308,11 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) struct vhost_virtqueue *vq = ubufs-vq; int cnt = atomic_read(ubufs-kref.refcount); + /* set len to mark this desc buffers done DMA */ + vq-heads[ubuf-desc].len = success ? + VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; + vhost_net_ubuf_put(ubufs); + /* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1 @@ -318,10 +323,6 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) */ if (cnt = 2 || !(cnt % 16)) vhost_poll_queue(vq-poll); - /* set len to mark this desc buffers done DMA */ - vq-heads[ubuf-desc].len = success ? - VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; - vhost_net_ubuf_put(ubufs); } /* Expects to be always run from workqueue - which acts as -- 1.7.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
