Re: [dpdk-dev] [PATCH] vchost: Notify application of ownership change

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2015-08-10 03:20, Ouyang, Changchun wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>> Sent: Saturday, August 8, 2015 2:43 PM
>> To: Ouyang, Changchun; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] vchost: Notify application of ownership
>> change
>>
>> On 2015-08-08 02:25, Ouyang, Changchun wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jan Kiszka
>>>> Sent: Saturday, August 8, 2015 1:21 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH] vchost: Notify application of ownership
>>>> change
>>>
>>> Vchost should be vhost in the title
>>
>> Oops. Unless I need to resend for some other reason, I guess the commit can
>> fix this up.
>>
>>>
>>>>
>>>> On VHOST_*_RESET_OWNER, we reinitialize the device but without
>>>> telling the application. That will cause crashes when it continues to
>>>> invoke vhost services on the device. Fix it by calling the
>>>> destruction hook if the device is still in use.
>>> What's your qemu version?
>>
>> git head, see my other reply for details.
>>
>>> Any validation work on this patch?
>>
>> What do you mean with this? Test cases? Or steps to reproduce? For the
>> latter, just fire up a recent qemu, let the guest enable the virtio device, then
>> reboot or simply terminate qemu.
> 
> Here, I mean test case, 
> Need make sure the change works on both qemu 2.4(with the reset commit in qemu) and qemu2.2/2.3(without the commit in qemu).
> 

I suspect, 2.2 and 2.3 stable have this fix queued already. If not, we
should trigger this. The previous versions were subtly broken and
shouldn't be used for production purposes.

>>
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> This is the surprisingly simple answer to my questions in
>>>> http://thread.gmane.org/gmane.comp.networking.dpdk.devel/22661.
>>>>
>>>>  lib/librte_vhost/virtio-net.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/lib/librte_vhost/virtio-net.c
>>>> b/lib/librte_vhost/virtio-net.c index
>>>> b520ec5..3c5b5b2 100644
>>>> --- a/lib/librte_vhost/virtio-net.c
>>>> +++ b/lib/librte_vhost/virtio-net.c
>>>> @@ -402,6 +402,9 @@ reset_owner(struct vhost_device_ctx ctx)
>>>>
>>>>  	ll_dev = get_config_ll_entry(ctx);
>>>>
>>>> +	if ((ll_dev->dev.flags & VIRTIO_DEV_RUNNING))
>>>> +		notify_ops->destroy_device(&ll_dev->dev);
>>>> +
>>>
>>> I am not sure whether destroy_device here will affect the second time
>> init_device(below) and new_device(after the reset) or not.
>>> Need validation.
>>
>> Cannot follow, what do you mean with "second time"? If the callback could
>> invoke something that causes cleanup_device to be called as well?
>> That's at least not the case with vhost-switch, but I'm far from being familiar
>> with the API to asses if that is possible in general.
> 
> RESET is often followed by a second time virtio device creation. 
> If you have chance to run testpmd with virtio PMD on guest, that would be that case:
> Call RESET, and then create virtio device again to make it work for packets rx/tx 

OK, need to dig into this, probably not the next days, though.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux