* [Qemu-devel] [PATCH 2/3] virtio: introduce virtqueue_discard()
2015-09-18 8:01 [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg() Jason Wang
@ 2015-09-18 8:01 ` Jason Wang
2015-09-18 8:01 ` [Qemu-devel] [PATCH 3/3] virtio-net: correctly drop truncated packets Jason Wang
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2015-09-18 8:01 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: Jason Wang, ppandit
This patch introduces virtqueue_discard() to discard a descriptor and
unmap the sgs. This will be used by the patch that will discard
descriptor when packet is truncated.
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/virtio/virtio.c | 7 +++++++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 9 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index eb8d5ca..d24ee80 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -265,6 +265,13 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
0, elem->out_sg[i].iov_len);
}
+void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
+ unsigned int len)
+{
+ vq->last_avail_idx--;
+ virtqueue_unmap_sg(vq, elem, len);
+}
+
void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
unsigned int len, unsigned int idx)
{
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 6201ee8..9d09115 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -146,6 +146,8 @@ void virtio_del_queue(VirtIODevice *vdev, int n);
void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
unsigned int len);
void virtqueue_flush(VirtQueue *vq, unsigned int count);
+void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
+ unsigned int len);
void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
unsigned int len, unsigned int idx);
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 3/3] virtio-net: correctly drop truncated packets
2015-09-18 8:01 [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg() Jason Wang
2015-09-18 8:01 ` [Qemu-devel] [PATCH 2/3] virtio: introduce virtqueue_discard() Jason Wang
@ 2015-09-18 8:01 ` Jason Wang
2015-09-24 14:51 ` [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg() Andrew James
2015-09-24 16:19 ` Andrew James
3 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2015-09-18 8:01 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: Jason Wang, ppandit
When packet is truncated during receiving, we drop the packets but
neither discard the descriptor nor add and signal used
descriptor. This will lead several issues:
- sg mappings are leaked
- rx will be stalled if a lots of packets were truncated
In order to be consistent with vhost, fix by discarding the descriptor
in this case.
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/virtio-net.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f72eebf..038a18b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1086,13 +1086,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
* must have consumed the complete packet.
* Otherwise, drop it. */
if (!n->mergeable_rx_bufs && offset < size) {
-#if 0
- error_report("virtio-net truncated non-mergeable packet: "
- "i %zd mergeable %d offset %zd, size %zd, "
- "guest hdr len %zd, host hdr len %zd",
- i, n->mergeable_rx_bufs,
- offset, size, n->guest_hdr_len, n->host_hdr_len);
-#endif
+ virtqueue_discard(q->rx_vq, &elem, total);
return size;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg()
2015-09-18 8:01 [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg() Jason Wang
2015-09-18 8:01 ` [Qemu-devel] [PATCH 2/3] virtio: introduce virtqueue_discard() Jason Wang
2015-09-18 8:01 ` [Qemu-devel] [PATCH 3/3] virtio-net: correctly drop truncated packets Jason Wang
@ 2015-09-24 14:51 ` Andrew James
2015-09-24 16:19 ` Andrew James
3 siblings, 0 replies; 6+ messages in thread
From: Andrew James @ 2015-09-24 14:51 UTC (permalink / raw)
To: qemu-devel
On 09/18/2015 02:01 AM, Jason Wang wrote:
> Factor out sg unmapping logic. This will be reused by the patch that
> can discard descriptor.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> hw/virtio/virtio.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0832db9..eb8d5ca 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -243,15 +243,12 @@ int virtio_queue_empty(VirtQueue *vq)
> return vring_avail_idx(vq) == vq->last_avail_idx;
> }
>
> -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> - unsigned int len, unsigned int idx)
> +static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
> + unsigned int len)
> {
> unsigned int offset;
> int i;
>
> - trace_virtqueue_fill(vq, elem, len, idx);
> -
> - offset = 0;
> for (i = 0; i < elem->in_num; i++) {
> size_t size = MIN(len - offset, elem->in_sg[i].iov_len);
>
Should the "offset = 0" really be dropped here? Seems like it ends
up uninitialized. GCC thinks it might too.
> @@ -266,6 +263,14 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> cpu_physical_memory_unmap(elem->out_sg[i].iov_base,
> elem->out_sg[i].iov_len,
> 0, elem->out_sg[i].iov_len);
> +}
> +
> +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> + unsigned int len, unsigned int idx)
> +{
> + trace_virtqueue_fill(vq, elem, len, idx);
> +
> + virtqueue_unmap_sg(vq, elem, len);
>
> idx = (idx + vring_used_idx(vq)) % vq->vring.num;
>
>
Thanks,
--
Andrew James
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg()
2015-09-18 8:01 [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg() Jason Wang
` (2 preceding siblings ...)
2015-09-24 14:51 ` [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg() Andrew James
@ 2015-09-24 16:19 ` Andrew James
2015-09-25 3:27 ` Jason Wang
3 siblings, 1 reply; 6+ messages in thread
From: Andrew James @ 2015-09-24 16:19 UTC (permalink / raw)
To: qemu-devel
On 09/18/2015 02:01 AM, Jason Wang wrote:
> Factor out sg unmapping logic. This will be reused by the patch that
> can discard descriptor.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> hw/virtio/virtio.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0832db9..eb8d5ca 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -243,15 +243,12 @@ int virtio_queue_empty(VirtQueue *vq)
> return vring_avail_idx(vq) == vq->last_avail_idx;
> }
>
> -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> - unsigned int len, unsigned int idx)
> +static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
> + unsigned int len)
> {
> unsigned int offset;
> int i;
>
> - trace_virtqueue_fill(vq, elem, len, idx);
> -
> - offset = 0;
> for (i = 0; i < elem->in_num; i++) {
> size_t size = MIN(len - offset, elem->in_sg[i].iov_len);
>
Should the "offset = 0" really be dropped here? Seems like it ends
up uninitialized. GCC thinks it might too.
> @@ -266,6 +263,14 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> cpu_physical_memory_unmap(elem->out_sg[i].iov_base,
> elem->out_sg[i].iov_len,
> 0, elem->out_sg[i].iov_len);
> +}
> +
> +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> + unsigned int len, unsigned int idx)
> +{
> + trace_virtqueue_fill(vq, elem, len, idx);
> +
> + virtqueue_unmap_sg(vq, elem, len);
>
> idx = (idx + vring_used_idx(vq)) % vq->vring.num;
>
>
Thanks,
--
Andrew James
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] virtio: introduce virtqueue_unmap_sg()
2015-09-24 16:19 ` Andrew James
@ 2015-09-25 3:27 ` Jason Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2015-09-25 3:27 UTC (permalink / raw)
To: Andrew James, qemu-devel
On 09/25/2015 12:19 AM, Andrew James wrote:
> On 09/18/2015 02:01 AM, Jason Wang wrote:
>> > Factor out sg unmapping logic. This will be reused by the patch that
>> > can discard descriptor.
>> >
>> > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>> > ---
>> > hw/virtio/virtio.c | 15 ++++++++++-----
>> > 1 file changed, 10 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> > index 0832db9..eb8d5ca 100644
>> > --- a/hw/virtio/virtio.c
>> > +++ b/hw/virtio/virtio.c
>> > @@ -243,15 +243,12 @@ int virtio_queue_empty(VirtQueue *vq)
>> > return vring_avail_idx(vq) == vq->last_avail_idx;
>> > }
>> >
>> > -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>> > - unsigned int len, unsigned int idx)
>> > +static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
>> > + unsigned int len)
>> > {
>> > unsigned int offset;
>> > int i;
>> >
>> > - trace_virtqueue_fill(vq, elem, len, idx);
>> > -
>> > - offset = 0;
>> > for (i = 0; i < elem->in_num; i++) {
>> > size_t size = MIN(len - offset, elem->in_sg[i].iov_len);
>> >
> Should the "offset = 0" really be dropped here?
Probably not.
> Seems like it ends
> up uninitialized. GCC thinks it might too.
>
Yes, will keep the offset initialization in V2.
Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread