Michael S. Tsirkin
2021-Oct-20 08:24 UTC
[PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed
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 at 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 at 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 > >
Xuan Zhuo
2021-Oct-20 11:07 UTC
[PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed
On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst at 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 at 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 at 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 weencounter 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 > > > >