Re: [dpdk-dev] [PATCH 4/6] vhost: workaround stale vring base

[Victor Kaplansky <vkaplans@redhat.com>]

----- Original Message -----
> From: "Yuanhan Liu" <yuanhan.liu@linux.intel.com>
> To: dev@dpdk.org
> Cc: "huawei xie" <huawei.xie@intel.com>, "Yuanhan Liu" <yuanhan.liu@linux.intel.com>, "Michael S. Tsirkin"
> <mst@redhat.com>
> Sent: Saturday, May 7, 2016 9:40:22 AM
> Subject: [dpdk-dev] [PATCH 4/6] vhost: workaround stale vring base
> 
> When DPDK app crashes (or quits, or gets killed), and when QEMU supports
> reconnecting (patches have been sent, not merged yet), a restart of DPDK
> app would get stale vring base from QEMU. That would break the kernel
> virtio net completely, making it non-work any more, unless a driver reset
> is done.
> 
> So, instead of getting the stale vring base from QEMU, Huawei suggested
> we could get a proper one from used->idx.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Suggested-by: "Xie, Huawei" <huawei.xie@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> 
> Note that a right way to handle reconnect from abnormal quit would be
> let the guest driver to initiate a reset when reconnect is detected
> from QEMU. As a reset from the virtio net driver would resets all virtio
> related meta datas, and trigger the vhost-user re-initiation again,
> therefore, it would make the reconnect work as expected.
> 
> What's unforunate is that driver reset on reconnect, as the term "driver"
> implies, need driver hackings, which could be a linux kernel virtio net
> driver, DPDK pmd driver, or even, the windows driver. Apparently, it will
> not work so far, or even for a long while, until we are adapted to the
> new kernel.
> 
> The fix (or more precisely, the workaround) from this patch would make
> reconnect work in most case, including the following two hypothesis that
> might corrupt virtio memory:
> 
> - vring_used_elem might be in stale state when we are in the middle of
>   updating used vring. Say, we might have updated the "id" field, but
>   leaving "len" untouched.
> 
> - vring desc buff might also be in stale state, when we are copying
>   data into it.
> 
> The reason it still works is that we haven't updated used->idx yet,
> which means guest driver will not start processing those buggy used
> entries. Therefore, no issues.
> 
> However, Michael claims some concerns: he made a good point: a crash
> is happening means some memory is corrupted, and it could be the virtio
> memory being corrupted. In such case, nothing will work without the
> reset.
> 
> But I would say, that an app in product use would rare crash, and even
> if it crashes, the chance that virtio memory being corrupted would be
> relatively smaller then. Besides that, it would work, say when the app
> is killed by ctrl-c or kill command. So, it works in the most cases.
> But still, I would say it's just a workaround so far.
> 
> On the other hand, without this patch, it would be 100% not working
> from abnormal quit. But with this patch, it works in most cases, just
> don't work in a rare case that when virtio memory is corrupted. Therefore,
> I'd say, it is still good to have, until we have a perfect solution.
> ---
>  lib/librte_vhost/virtio-net.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index c88aaa3..df103aa 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -560,6 +560,14 @@ vhost_set_vring_addr(int vid, struct vhost_vring_addr
> *addr)
>  		return -1;
>  	}
>  
> +	if (vq->last_used_idx != vq->used->idx) {
> +		RTE_LOG(WARNING, VHOST_CONFIG,
> +			"last_used_idx (%u) and vq->used->idx (%u) mismatch\n",

I agree with this approach. I just would add to the log message that last_user_idx
have overrode by used_idx and some packets may be dropped.

> +			vq->last_used_idx, vq->used->idx);
> +		vq->last_used_idx     = vq->used->idx;
> +		vq->last_used_idx_res = vq->used->idx;
> +	}
> +
>  	vq->log_guest_addr = addr->log_guest_addr;
>  
>  	LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
> --
> 1.9.0
> 
>