On Tue, Oct 27, 2015 at 08:46:48AM +0000, Xie, Huawei wrote:
> On 10/27/2015 4:39 PM, Yuanhan Liu wrote:
> > On Tue, Oct 27, 2015 at 08:24:00AM +0000, Xie, Huawei wrote:
> >> On 10/27/2015 3:52 PM, Tetsuya Mukawa wrote:
> >>> The patch fixes wrong handling of virtqueue array index when
> >>> GET_VRING_BASE message comes.
> >>> The vhost backend will receive the message per virtqueue.
> >>> Also we should call a destroy callback handler when both RXQ
> >>> and TXQ receives the message.
> >>>
> >>> Signed-off-by: Tetsuya Mukawa <mukawa at igel.co.jp>
> >>> ---
> >>> lib/librte_vhost/vhost_user/virtio-net-user.c | 20 ++++++++++----------
> >>> 1 file changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c
> >>> b/lib/librte_vhost/vhost_user/virtio-net-user.c
> >>> index a998ad8..99c075f 100644
> >>> --- a/lib/librte_vhost/vhost_user/virtio-net-user.c
> >>> +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
> >>> @@ -283,12 +283,10 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> >>> struct vhost_vring_state *state)
> >>> {
> >>> struct virtio_net *dev = get_device(ctx);
> >>> + uint16_t base_idx = state->index / VIRTIO_QNUM * VIRTIO_QNUM;
> >>>
> >>> if (dev == NULL)
> >>> return -1;
> >>> - /* We have to stop the queue (virtio) if it is running. */
> >>> - if (dev->flags & VIRTIO_DEV_RUNNING)
> >>> - notify_ops->destroy_device(dev);
> >> Hi Tetsuya:
> >> I don't understand why we move it to the end of the function.
> >> If we don't tell the application to remove the virtio device from the
> > As you stated, he just moved it to the end of the function: it
> > still does invoke notfiy_ops->destroy_device() in the end.
> The problem is before calling destroy_device, we shouldn't modify the
> virtio_net data structure as data plane is also using it.
Right then, we may shoud not move it in the end.
> >
> > And the reason he moved it to the end is he want to invoke the
> > callback just when the second GET_VRING_BASE message is received
> > for the queue pair.
> Don't get it. What issue it fixes?
I guess Tetsuya thinks that'd be a more proper time to invoke the
callback, but in fact, it's not, as we have MQ enabled :)
--yliu
> > And while thinking twice, it's not necessary,
> > as we will do the "flags & VIRTIO_DEV_RUNNING" check first, it
> > doesn't matter on which virt queue we invoke the callback.
> >
> >
> > --yliu
> >
> >> data plane, then the vhost application is still operating on that
> >> device, we shouldn't do anything to the virtio_net device.
> >> For this case, as vhost doesn't use kickfd, it will not cause issue, but
> >> i think it is best practice firstly to remove it from data plan through
> >> destroy_device.
> >>
> >> I think we could call destroy_device the first time we receive this
> >> message. Currently we don't have per queue granularity control to only
> >> remove one queue from data plane.
> >>
> >> I am Okay to only close the kickfd for the specified queue index.
> >>
> >> Btw, do you meet issue with previous implementation?
> >>>
> >>> /* Here we are safe to get the last used index */
> >>> ops->get_vring_base(ctx, state->index, state);
> >>> @@ -300,15 +298,17 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> >>> * sent and only sent in vhost_vring_stop.
> >>> * TODO: cleanup the vring, it isn't usable since here.
> >>> */
> >>> - if (dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd >= 0) {
> >>> - close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
> >>> - dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
> >>> - }
> >>> - if (dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd >= 0) {
> >>> - close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
> >>> - dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
> >>> + if (dev->virtqueue[state->index]->kickfd >= 0) {
> >>> + close(dev->virtqueue[state->index]->kickfd);
> >>> + dev->virtqueue[state->index]->kickfd = -1;
> >>> }
> >>>
> >>> + /* We have to stop the queue (virtio) if it is running. */
> >>> + if ((dev->flags & VIRTIO_DEV_RUNNING) &&
> >>> + (dev->virtqueue[base_idx + VIRTIO_RXQ]->kickfd == -1) &&
> >>> + (dev->virtqueue[base_idx + VIRTIO_TXQ]->kickfd == -1))
> >>> + notify_ops->destroy_device(dev);
> >>> +
> >>> return 0;
> >>> }
> >>>
>
