* [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect
@ 2021-10-19 11:52 Xuan Zhuo
2021-10-19 11:52 ` [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed Xuan Zhuo
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Xuan Zhuo @ 2021-10-19 11:52 UTC (permalink / raw)
To: Michael S. Tsirkin, virtualization; +Cc: David S. Miller, Tiwei Bie
In the case of packed, use indirect desc, since desc is allocated by
kmalloc_array(), we should check whether its return value is NULL.
v4:
Inside the #2 patch, virtqueue_add_indirect_packed() return -EAGAIN when
desc == NULL.
v3:
Update commit message of the #1 patch.
v2:
Separate the style fix into a single patch.
Xuan Zhuo (2):
virtio_ring: fix style of virtqueue_add_indirect_packed
virtio_ring: check desc == NULL when using indirect with packed
drivers/virtio/virtio_ring.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
--
2.31.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed 2021-10-19 11:52 [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Xuan Zhuo @ 2021-10-19 11:52 ` Xuan Zhuo 2021-10-19 13:22 ` Michael S. Tsirkin 2021-10-19 11:52 ` [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed Xuan Zhuo 2021-10-19 13:23 ` [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Michael S. Tsirkin 2 siblings, 1 reply; 12+ messages in thread From: Xuan Zhuo @ 2021-10-19 11:52 UTC (permalink / raw) To: Michael S. Tsirkin, virtualization; +Cc: David S. Miller, Tiwei Bie Align the arguments of virtqueue_add_indirect_packed() to the open ( to make it look prettier. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio_ring.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index dd95dfd85e98..91a46c4da87d 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1050,12 +1050,12 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg, } static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, - struct scatterlist *sgs[], - unsigned int total_sg, - unsigned int out_sgs, - unsigned int in_sgs, - void *data, - gfp_t gfp) + struct scatterlist *sgs[], + unsigned int total_sg, + unsigned int out_sgs, + unsigned int in_sgs, + void *data, + gfp_t gfp) { struct vring_packed_desc *desc; struct scatterlist *sg; -- 2.31.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed 2021-10-19 11:52 ` [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed Xuan Zhuo @ 2021-10-19 13:22 ` Michael S. Tsirkin 0 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2021-10-19 13:22 UTC (permalink / raw) To: Xuan Zhuo; +Cc: David S. Miller, Tiwei Bie, virtualization On Tue, Oct 19, 2021 at 07:52:34PM +0800, Xuan Zhuo wrote: > Align the arguments of virtqueue_add_indirect_packed() to the open ( to > make it look prettier. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > Acked-by: Jason Wang <jasowang@redhat.com> ok now update subject to match pls > --- > drivers/virtio/virtio_ring.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index dd95dfd85e98..91a46c4da87d 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1050,12 +1050,12 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg, > } > > static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > - struct scatterlist *sgs[], > - unsigned int total_sg, > - unsigned int out_sgs, > - unsigned int in_sgs, > - void *data, > - gfp_t gfp) > + struct scatterlist *sgs[], > + unsigned int total_sg, > + unsigned int out_sgs, > + unsigned int in_sgs, > + void *data, > + gfp_t gfp) > { > struct vring_packed_desc *desc; > struct scatterlist *sg; > -- > 2.31.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-19 11:52 [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Xuan Zhuo 2021-10-19 11:52 ` [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed Xuan Zhuo @ 2021-10-19 11:52 ` Xuan Zhuo 2021-10-19 13:21 ` Michael S. Tsirkin 2021-10-19 13:24 ` Michael S. Tsirkin 2021-10-19 13:23 ` [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Michael S. Tsirkin 2 siblings, 2 replies; 12+ messages in thread From: Xuan Zhuo @ 2021-10-19 11:52 UTC (permalink / raw) To: Michael S. Tsirkin, virtualization; +Cc: David S. Miller, Tiwei Bie When using indirect with packed, we don't check for allocation failures. This patch checks that and fall back on direct. Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 91a46c4da87d..44a03b6e4dc4 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, head = vq->packed.next_avail_idx; desc = alloc_indirect_packed(total_sg, gfp); + if (!desc) + /* fall back on direct */ + return -EAGAIN; if (unlikely(vq->vq.num_free < 1)) { pr_debug("Can't add buf len 1 - avail = 0\n"); @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, unsigned int i, n, c, descs_used, err_idx; __le16 head_flags, flags; u16 head, id, prev, curr, avail_used_flags; + int err; START_USE(vq); @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, BUG_ON(total_sg == 0); - if (virtqueue_use_indirect(_vq, total_sg)) - return virtqueue_add_indirect_packed(vq, sgs, total_sg, - out_sgs, in_sgs, data, gfp); + if (virtqueue_use_indirect(_vq, total_sg)) { + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, + in_sgs, data, gfp); + if (err != -EAGAIN) + return err; + } head = vq->packed.next_avail_idx; avail_used_flags = vq->packed.avail_used_flags; -- 2.31.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-19 11:52 ` [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed Xuan Zhuo @ 2021-10-19 13:21 ` Michael S. Tsirkin 2021-10-20 2:19 ` Xuan Zhuo 2021-10-19 13:24 ` Michael S. Tsirkin 1 sibling, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2021-10-19 13:21 UTC (permalink / raw) To: Xuan Zhuo; +Cc: David S. Miller, Tiwei Bie, virtualization On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > When using indirect with packed, we don't check for allocation failures. > This patch checks that and fall back on direct. > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 91a46c4da87d..44a03b6e4dc4 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > head = vq->packed.next_avail_idx; > desc = alloc_indirect_packed(total_sg, gfp); > + if (!desc) > + /* fall back on direct */ this comment belongs in virtqueue_add_packed right after return. > + return -EAGAIN; > -ENOMEM surely? > if (unlikely(vq->vq.num_free < 1)) { > pr_debug("Can't add buf len 1 - avail = 0\n"); > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > unsigned int i, n, c, descs_used, err_idx; > __le16 head_flags, flags; > u16 head, id, prev, curr, avail_used_flags; > + int err; > > START_USE(vq); > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > BUG_ON(total_sg == 0); > > - if (virtqueue_use_indirect(_vq, total_sg)) > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > - out_sgs, in_sgs, data, gfp); > + if (virtqueue_use_indirect(_vq, total_sg)) { > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > + in_sgs, data, gfp); > + if (err != -EAGAIN) > + return err; > + } > > head = vq->packed.next_avail_idx; > avail_used_flags = vq->packed.avail_used_flags; > -- > 2.31.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-19 13:21 ` Michael S. Tsirkin @ 2021-10-20 2:19 ` Xuan Zhuo 2021-10-20 8:24 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Xuan Zhuo @ 2021-10-20 2:19 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: David S. Miller, Tiwei Bie, virtualization On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > > When using indirect with packed, we don't check for allocation failures. > > This patch checks that and fall back on direct. > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 91a46c4da87d..44a03b6e4dc4 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > > > head = vq->packed.next_avail_idx; > > desc = alloc_indirect_packed(total_sg, gfp); > > + if (!desc) > > + /* fall back on direct */ > > this comment belongs in virtqueue_add_packed right after > return. > > > + return -EAGAIN; > > > > -ENOMEM surely? virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we have to choose a different error code. Thanks. > > > if (unlikely(vq->vq.num_free < 1)) { > > pr_debug("Can't add buf len 1 - avail = 0\n"); > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > unsigned int i, n, c, descs_used, err_idx; > > __le16 head_flags, flags; > > u16 head, id, prev, curr, avail_used_flags; > > + int err; > > > > START_USE(vq); > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > BUG_ON(total_sg == 0); > > > > - if (virtqueue_use_indirect(_vq, total_sg)) > > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > > - out_sgs, in_sgs, data, gfp); > > + if (virtqueue_use_indirect(_vq, total_sg)) { > > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > > + in_sgs, data, gfp); > > + if (err != -EAGAIN) > > + return err; > > + } > > > > head = vq->packed.next_avail_idx; > > avail_used_flags = vq->packed.avail_used_flags; > > -- > > 2.31.0 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-20 2:19 ` Xuan Zhuo @ 2021-10-20 8:24 ` Michael S. Tsirkin 2021-10-20 11:07 ` Xuan Zhuo 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2021-10-20 8:24 UTC (permalink / raw) To: Xuan Zhuo; +Cc: David S. Miller, Tiwei Bie, virtualization On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote: > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > > > When using indirect with packed, we don't check for allocation failures. > > > This patch checks that and fall back on direct. > > > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > --- > > > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 91a46c4da87d..44a03b6e4dc4 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > > > > > head = vq->packed.next_avail_idx; > > > desc = alloc_indirect_packed(total_sg, gfp); > > > + if (!desc) > > > + /* fall back on direct */ > > > > this comment belongs in virtqueue_add_packed right after > > return. > > > > > + return -EAGAIN; > > > > > > > -ENOMEM surely? > > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we > have to choose a different error code. > > Thanks. That's exactly the point. Why would we want to recover from allocation failure but not DMA map failure? > > > > > if (unlikely(vq->vq.num_free < 1)) { > > > pr_debug("Can't add buf len 1 - avail = 0\n"); > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > unsigned int i, n, c, descs_used, err_idx; > > > __le16 head_flags, flags; > > > u16 head, id, prev, curr, avail_used_flags; > > > + int err; > > > > > > START_USE(vq); > > > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > > > BUG_ON(total_sg == 0); > > > > > > - if (virtqueue_use_indirect(_vq, total_sg)) > > > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > > > - out_sgs, in_sgs, data, gfp); > > > + if (virtqueue_use_indirect(_vq, total_sg)) { > > > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > > > + in_sgs, data, gfp); > > > + if (err != -EAGAIN) > > > + return err; > > > + } > > > > > > head = vq->packed.next_avail_idx; > > > avail_used_flags = vq->packed.avail_used_flags; > > > -- > > > 2.31.0 > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-20 8:24 ` Michael S. Tsirkin @ 2021-10-20 11:07 ` Xuan Zhuo 2021-10-20 14:52 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Xuan Zhuo @ 2021-10-20 11:07 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: David S. Miller, Tiwei Bie, virtualization On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote: > > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > > > > When using indirect with packed, we don't check for allocation failures. > > > > This patch checks that and fall back on direct. > > > > > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > --- > > > > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > index 91a46c4da87d..44a03b6e4dc4 100644 > > > > --- a/drivers/virtio/virtio_ring.c > > > > +++ b/drivers/virtio/virtio_ring.c > > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > > > > > > > head = vq->packed.next_avail_idx; > > > > desc = alloc_indirect_packed(total_sg, gfp); > > > > + if (!desc) > > > > + /* fall back on direct */ > > > > > > this comment belongs in virtqueue_add_packed right after > > > return. > > > > > > > + return -EAGAIN; > > > > > > > > > > -ENOMEM surely? > > > > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we > > have to choose a different error code. > > > > Thanks. > > That's exactly the point. Why would we want to recover from allocation > failure but not DMA map failure? From indirect to direct mode, there is no need to allocate memory, so if we encounter memory allocation failure, we can fall back from indirect to direct mode. Memory allocation failure is a temporary problem. And if you encounter a dma mmap error, this situation should be notified to the upper layer. Thanks. > > > > > > > > if (unlikely(vq->vq.num_free < 1)) { > > > > pr_debug("Can't add buf len 1 - avail = 0\n"); > > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > unsigned int i, n, c, descs_used, err_idx; > > > > __le16 head_flags, flags; > > > > u16 head, id, prev, curr, avail_used_flags; > > > > + int err; > > > > > > > > START_USE(vq); > > > > > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > > > > > BUG_ON(total_sg == 0); > > > > > > > > - if (virtqueue_use_indirect(_vq, total_sg)) > > > > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > > > > - out_sgs, in_sgs, data, gfp); > > > > + if (virtqueue_use_indirect(_vq, total_sg)) { > > > > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > > > > + in_sgs, data, gfp); > > > > + if (err != -EAGAIN) > > > > + return err; > > > > + } > > > > > > > > head = vq->packed.next_avail_idx; > > > > avail_used_flags = vq->packed.avail_used_flags; > > > > -- > > > > 2.31.0 > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-20 11:07 ` Xuan Zhuo @ 2021-10-20 14:52 ` Michael S. Tsirkin 2021-10-21 1:54 ` Xuan Zhuo 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2021-10-20 14:52 UTC (permalink / raw) To: Xuan Zhuo; +Cc: David S. Miller, Tiwei Bie, virtualization On Wed, Oct 20, 2021 at 07:07:43PM +0800, Xuan Zhuo wrote: > On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote: > > > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > > > > > When using indirect with packed, we don't check for allocation failures. > > > > > This patch checks that and fall back on direct. > > > > > > > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > --- > > > > > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > index 91a46c4da87d..44a03b6e4dc4 100644 > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > > > > > > > > > head = vq->packed.next_avail_idx; > > > > > desc = alloc_indirect_packed(total_sg, gfp); > > > > > + if (!desc) > > > > > + /* fall back on direct */ > > > > > > > > this comment belongs in virtqueue_add_packed right after > > > > return. > > > > > > > > > + return -EAGAIN; > > > > > > > > > > > > > -ENOMEM surely? > > > > > > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we > > > have to choose a different error code. > > > > > > Thanks. > > > > That's exactly the point. Why would we want to recover from allocation > > failure but not DMA map failure? > > >From indirect to direct mode, there is no need to allocate memory, so if we > encounter memory allocation failure, we can fall back from indirect to direct > mode. Memory allocation failure is a temporary problem. > > And if you encounter a dma mmap error, this situation should be notified to the > upper layer. > > Thanks. Why did dma map fail? A common reason is precisely that we are running out of buffer space. Using direct which does not need extra buffer space thus has a chance to work. > > > > > > > > > > > if (unlikely(vq->vq.num_free < 1)) { > > > > > pr_debug("Can't add buf len 1 - avail = 0\n"); > > > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > > unsigned int i, n, c, descs_used, err_idx; > > > > > __le16 head_flags, flags; > > > > > u16 head, id, prev, curr, avail_used_flags; > > > > > + int err; > > > > > > > > > > START_USE(vq); > > > > > > > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > > > > > > > BUG_ON(total_sg == 0); > > > > > > > > > > - if (virtqueue_use_indirect(_vq, total_sg)) > > > > > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > > > > > - out_sgs, in_sgs, data, gfp); > > > > > + if (virtqueue_use_indirect(_vq, total_sg)) { > > > > > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > > > > > + in_sgs, data, gfp); > > > > > + if (err != -EAGAIN) > > > > > + return err; > > > > > + } > > > > > > > > > > head = vq->packed.next_avail_idx; > > > > > avail_used_flags = vq->packed.avail_used_flags; > > > > > -- > > > > > 2.31.0 > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-20 14:52 ` Michael S. Tsirkin @ 2021-10-21 1:54 ` Xuan Zhuo 0 siblings, 0 replies; 12+ messages in thread From: Xuan Zhuo @ 2021-10-21 1:54 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: David S. Miller, Tiwei Bie, virtualization On Wed, 20 Oct 2021 10:52:24 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Oct 20, 2021 at 07:07:43PM +0800, Xuan Zhuo wrote: > > On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > > > On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote: > > > > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > > > > > > When using indirect with packed, we don't check for allocation failures. > > > > > > This patch checks that and fall back on direct. > > > > > > > > > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > > --- > > > > > > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > > > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > > index 91a46c4da87d..44a03b6e4dc4 100644 > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > > > > > > > > > > > head = vq->packed.next_avail_idx; > > > > > > desc = alloc_indirect_packed(total_sg, gfp); > > > > > > + if (!desc) > > > > > > + /* fall back on direct */ > > > > > > > > > > this comment belongs in virtqueue_add_packed right after > > > > > return. > > > > > > > > > > > + return -EAGAIN; > > > > > > > > > > > > > > > > -ENOMEM surely? > > > > > > > > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we > > > > have to choose a different error code. > > > > > > > > Thanks. > > > > > > That's exactly the point. Why would we want to recover from allocation > > > failure but not DMA map failure? > > > > >From indirect to direct mode, there is no need to allocate memory, so if we > > encounter memory allocation failure, we can fall back from indirect to direct > > mode. Memory allocation failure is a temporary problem. > > > > And if you encounter a dma mmap error, this situation should be notified to the > > upper layer. > > > > Thanks. > > Why did dma map fail? > A common reason is precisely that we are running > out of buffer space. Using direct which does not > need extra buffer space thus has a chance to work. It turns out that this is the case, then I have no problem. Thanks. > > > > > > > > > > > > > > > > if (unlikely(vq->vq.num_free < 1)) { > > > > > > pr_debug("Can't add buf len 1 - avail = 0\n"); > > > > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > > > unsigned int i, n, c, descs_used, err_idx; > > > > > > __le16 head_flags, flags; > > > > > > u16 head, id, prev, curr, avail_used_flags; > > > > > > + int err; > > > > > > > > > > > > START_USE(vq); > > > > > > > > > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > > > > > > > > > > BUG_ON(total_sg == 0); > > > > > > > > > > > > - if (virtqueue_use_indirect(_vq, total_sg)) > > > > > > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > > > > > > - out_sgs, in_sgs, data, gfp); > > > > > > + if (virtqueue_use_indirect(_vq, total_sg)) { > > > > > > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > > > > > > + in_sgs, data, gfp); > > > > > > + if (err != -EAGAIN) > > > > > > + return err; > > > > > > + } > > > > > > > > > > > > head = vq->packed.next_avail_idx; > > > > > > avail_used_flags = vq->packed.avail_used_flags; > > > > > > -- > > > > > > 2.31.0 > > > > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed 2021-10-19 11:52 ` [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed Xuan Zhuo 2021-10-19 13:21 ` Michael S. Tsirkin @ 2021-10-19 13:24 ` Michael S. Tsirkin 1 sibling, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2021-10-19 13:24 UTC (permalink / raw) To: Xuan Zhuo; +Cc: David S. Miller, Tiwei Bie, virtualization On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote: > When using indirect with packed, we don't check for allocation failures. > This patch checks that and fall back on direct. > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Was a crash observed with this? It seems quite likely ... > --- > drivers/virtio/virtio_ring.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 91a46c4da87d..44a03b6e4dc4 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, > > head = vq->packed.next_avail_idx; > desc = alloc_indirect_packed(total_sg, gfp); > + if (!desc) > + /* fall back on direct */ > + return -EAGAIN; > > if (unlikely(vq->vq.num_free < 1)) { > pr_debug("Can't add buf len 1 - avail = 0\n"); > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > unsigned int i, n, c, descs_used, err_idx; > __le16 head_flags, flags; > u16 head, id, prev, curr, avail_used_flags; > + int err; > > START_USE(vq); > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > BUG_ON(total_sg == 0); > > - if (virtqueue_use_indirect(_vq, total_sg)) > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, > - out_sgs, in_sgs, data, gfp); > + if (virtqueue_use_indirect(_vq, total_sg)) { > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, > + in_sgs, data, gfp); > + if (err != -EAGAIN) > + return err; > + } > > head = vq->packed.next_avail_idx; > avail_used_flags = vq->packed.avail_used_flags; > -- > 2.31.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect 2021-10-19 11:52 [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Xuan Zhuo 2021-10-19 11:52 ` [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed Xuan Zhuo 2021-10-19 11:52 ` [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed Xuan Zhuo @ 2021-10-19 13:23 ` Michael S. Tsirkin 2 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2021-10-19 13:23 UTC (permalink / raw) To: Xuan Zhuo; +Cc: David S. Miller, Tiwei Bie, virtualization On Tue, Oct 19, 2021 at 07:52:33PM +0800, Xuan Zhuo wrote: > In the case of packed, use indirect desc, since desc is allocated by > kmalloc_array(), we should check whether its return value is NULL. > a better description here: fix theoretical issues in virtio_ring (so I'm guessing - or did you observe any null pointer dereferences?) > v4: > Inside the #2 patch, virtqueue_add_indirect_packed() return -EAGAIN when > desc == NULL. > > v3: > Update commit message of the #1 patch. > > v2: > Separate the style fix into a single patch. > > > Xuan Zhuo (2): > virtio_ring: fix style of virtqueue_add_indirect_packed > virtio_ring: check desc == NULL when using indirect with packed > > drivers/virtio/virtio_ring.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > -- > 2.31.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-21 1:56 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-19 11:52 [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Xuan Zhuo 2021-10-19 11:52 ` [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed Xuan Zhuo 2021-10-19 13:22 ` Michael S. Tsirkin 2021-10-19 11:52 ` [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed Xuan Zhuo 2021-10-19 13:21 ` Michael S. Tsirkin 2021-10-20 2:19 ` Xuan Zhuo 2021-10-20 8:24 ` Michael S. Tsirkin 2021-10-20 11:07 ` Xuan Zhuo 2021-10-20 14:52 ` Michael S. Tsirkin 2021-10-21 1:54 ` Xuan Zhuo 2021-10-19 13:24 ` Michael S. Tsirkin 2021-10-19 13:23 ` [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox