| Message ID | 1473261649-31465-3-git-send-email-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote: > From: Stefan Hajnoczi <stefanha@redhat.com> > > virtqueue_discard() requires a VirtQueueElement but virtio-balloon does > not migrate its in-use element. Introduce a new function that is > similar to virtqueue_discard() but doesn't require a VirtQueueElement. > > This will allow virtio-balloon to access element again after migration > with the usual proviso that the guest may have modified the vring since > last time. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Roman Kagan <rkagan@virtuozzo.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Ladi Prosek <lprosek@redhat.com> > --- > hw/virtio/virtio.c | 22 ++++++++++++++++++++++ > include/hw/virtio/virtio.h | 1 + > 2 files changed, 23 insertions(+) Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> One question though: > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 74c085c..3de6029 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, > virtqueue_unmap_sg(vq, elem, len); > } > > +/* virtqueue_rewind: > + * @vq: The #VirtQueue > + * @num: Number of elements to push back > + * > + * Pretend that elements weren't popped from the virtqueue. The next > + * virtqueue_pop() will refetch the oldest element. > + * > + * Use virtqueue_discard() instead if you have a VirtQueueElement. > + * > + * Returns: true on success, false if @num is greater than the number of in use > + * elements. > + */ > +bool virtqueue_rewind(VirtQueue *vq, unsigned int num) > +{ > + if (num > vq->inuse) { > + return false; > + } > + vq->last_avail_idx -= num; > + vq->inuse -= num; > + return true; > +} > + Presumably you envision rewinding by something other than ->inuse. Do you have in mind a usecase for that, or is it just a matter of API symmetry or whatnot? Thanks, Roman.
On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <rkagan@virtuozzo.com> wrote: > On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote: >> From: Stefan Hajnoczi <stefanha@redhat.com> >> >> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does >> not migrate its in-use element. Introduce a new function that is >> similar to virtqueue_discard() but doesn't require a VirtQueueElement. >> >> This will allow virtio-balloon to access element again after migration >> with the usual proviso that the guest may have modified the vring since >> last time. >> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: Roman Kagan <rkagan@virtuozzo.com> >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> >> --- >> hw/virtio/virtio.c | 22 ++++++++++++++++++++++ >> include/hw/virtio/virtio.h | 1 + >> 2 files changed, 23 insertions(+) > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > > One question though: > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 74c085c..3de6029 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, >> virtqueue_unmap_sg(vq, elem, len); >> } >> >> +/* virtqueue_rewind: >> + * @vq: The #VirtQueue >> + * @num: Number of elements to push back >> + * >> + * Pretend that elements weren't popped from the virtqueue. The next >> + * virtqueue_pop() will refetch the oldest element. >> + * >> + * Use virtqueue_discard() instead if you have a VirtQueueElement. >> + * >> + * Returns: true on success, false if @num is greater than the number of in use >> + * elements. >> + */ >> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num) >> +{ >> + if (num > vq->inuse) { >> + return false; >> + } >> + vq->last_avail_idx -= num; >> + vq->inuse -= num; >> + return true; >> +} >> + > > Presumably you envision rewinding by something other than ->inuse. Do > you have in mind a usecase for that, or is it just a matter of API > symmetry or whatnot? It may not always be correct to rewind by ->inuse. The elements that are in use do not necessarily have to be at the end of the available ring. Example: elem1 = virtqueue_pop(); elem2 = virtqueue_pop(); elem3 = virtqueue_pop(); virtqueue_push(elem2); Now inuse == 2 but rewinding by 2 would be incorrect because elem2 has already been pushed to the used ring. So it is a dangerous API, which I believe is why Stefan added the num parameter even though it is currently not needed. Thanks, Ladi
On Thu, Sep 08, 2016 at 09:30:13AM +0200, Ladi Prosek wrote: > On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <rkagan@virtuozzo.com> wrote: > > On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote: > >> From: Stefan Hajnoczi <stefanha@redhat.com> > >> > >> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does > >> not migrate its in-use element. Introduce a new function that is > >> similar to virtqueue_discard() but doesn't require a VirtQueueElement. > >> > >> This will allow virtio-balloon to access element again after migration > >> with the usual proviso that the guest may have modified the vring since > >> last time. > >> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Cc: Roman Kagan <rkagan@virtuozzo.com> > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> > >> --- > >> hw/virtio/virtio.c | 22 ++++++++++++++++++++++ > >> include/hw/virtio/virtio.h | 1 + > >> 2 files changed, 23 insertions(+) > > > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > > > > One question though: > > > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> index 74c085c..3de6029 100644 > >> --- a/hw/virtio/virtio.c > >> +++ b/hw/virtio/virtio.c > >> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, > >> virtqueue_unmap_sg(vq, elem, len); > >> } > >> > >> +/* virtqueue_rewind: > >> + * @vq: The #VirtQueue > >> + * @num: Number of elements to push back > >> + * > >> + * Pretend that elements weren't popped from the virtqueue. The next > >> + * virtqueue_pop() will refetch the oldest element. > >> + * > >> + * Use virtqueue_discard() instead if you have a VirtQueueElement. > >> + * > >> + * Returns: true on success, false if @num is greater than the number of in use > >> + * elements. > >> + */ > >> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num) > >> +{ > >> + if (num > vq->inuse) { > >> + return false; > >> + } > >> + vq->last_avail_idx -= num; > >> + vq->inuse -= num; > >> + return true; > >> +} > >> + > > > > Presumably you envision rewinding by something other than ->inuse. Do > > you have in mind a usecase for that, or is it just a matter of API > > symmetry or whatnot? > > It may not always be correct to rewind by ->inuse. The elements that > are in use do not necessarily have to be at the end of the available > ring. Example: > > elem1 = virtqueue_pop(); > elem2 = virtqueue_pop(); > elem3 = virtqueue_pop(); > > virtqueue_push(elem2); > > Now inuse == 2 but rewinding by 2 would be incorrect because elem2 has > already been pushed to the used ring. > > So it is a dangerous API, which I believe is why Stefan added the num > parameter even though it is currently not needed. The function could be hard-coded to rewind just 1 element. I wanted to make it easy for a device to rewind the last n elements, but this functionality isn't used. Stefan
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 74c085c..3de6029 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, virtqueue_unmap_sg(vq, elem, len); } +/* virtqueue_rewind: + * @vq: The #VirtQueue + * @num: Number of elements to push back + * + * Pretend that elements weren't popped from the virtqueue. The next + * virtqueue_pop() will refetch the oldest element. + * + * Use virtqueue_discard() instead if you have a VirtQueueElement. + * + * Returns: true on success, false if @num is greater than the number of in use + * elements. + */ +bool virtqueue_rewind(VirtQueue *vq, unsigned int num) +{ + if (num > vq->inuse) { + return false; + } + vq->last_avail_idx -= num; + vq->inuse -= num; + return true; +} + 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 d2490c1..f05559d 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -154,6 +154,7 @@ void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, void virtqueue_flush(VirtQueue *vq, unsigned int count); void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len); +bool virtqueue_rewind(VirtQueue *vq, unsigned int num); void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len, unsigned int idx);