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

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

[Jan Kiszka]

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.

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);
+
 	cleanup_device(&ll_dev->dev);
 	init_device(&ll_dev->dev);

-- 
2.1.4

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

[Ouyang, Changchun]

> -----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

> 
> 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?
Any validation work on this patch?
> 
> 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.

>  	cleanup_device(&ll_dev->dev);
>  	init_device(&ll_dev->dev);
> 
> --
> 2.1.4

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

[Jan Kiszka]

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.

>>
>> 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.

Jan

> 
>>  	cleanup_device(&ll_dev->dev);
>>  	init_device(&ll_dev->dev);
>>
>> --
>> 2.1.4


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

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

[Ouyang, Changchun]

> -----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).

> 
> >>
> >> 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 

> 
> Jan
> 
> >
> >>  	cleanup_device(&ll_dev->dev);
> >>  	init_device(&ll_dev->dev);
> >>
> >> --
> >> 2.1.4
> 
> 
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate
> Competence Center Embedded Linux

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

[Jan Kiszka]

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

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

[Xie, Huawei]

On 8/8/2015 1:21 AM, Jan Kiszka wrote:
> 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.
>
> 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);

To me this patch makes sense here.
Whether RESET_OWNER is really needed is another question. Whenever the
vhost itself needs to process the vhost device, we need to notify the
switch application to remove it from data plane.
> +
>  	cleanup_device(&ll_dev->dev);
>  	init_device(&ll_dev->dev);
>

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

[Jan Kiszka]

On 2015-08-12 05:34, Xie, Huawei wrote:
> On 8/8/2015 1:21 AM, Jan Kiszka wrote:
>> 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.
>>
>> 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);
> 
> To me this patch makes sense here.
> Whether RESET_OWNER is really needed is another question. Whenever the

Is there a better way for the client for telling vhost to stop
delivering, ie. touching the client's memory?

Jan

> vhost itself needs to process the vhost device, we need to notify the
> switch application to remove it from data plane.
>> +
>>  	cleanup_device(&ll_dev->dev);
>>  	init_device(&ll_dev->dev);
>>
> 

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

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

[Thomas Monjalon]

2015-08-12 03:34, Xie, Huawei:
> On 8/8/2015 1:21 AM, Jan Kiszka wrote:
> > 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.
[...]
> > --- 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);
> 
> To me this patch makes sense here.
> Whether RESET_OWNER is really needed is another question. Whenever the
> vhost itself needs to process the vhost device, we need to notify the
> switch application to remove it from data plane.

Huawei,
some patches have been accepted for RESET_OWNER management.
Is this patch obsolete?

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

[Tetsuya Mukawa]

On 2015/10/25 2:16, Thomas Monjalon wrote:
> 2015-08-12 03:34, Xie, Huawei:
>> On 8/8/2015 1:21 AM, Jan Kiszka wrote:
>>> 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.
> [...]
>>> --- 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);
>> To me this patch makes sense here.
>> Whether RESET_OWNER is really needed is another question. Whenever the
>> vhost itself needs to process the vhost device, we need to notify the
>> switch application to remove it from data plane.
> Huawei,
> some patches have been accepted for RESET_OWNER management.
> Is this patch obsolete?

Hi Yuanhan and Huawei,

I also have the same question. Do we have a patch for this issue?

Today, I've download Yuanhan's multiple queues patches and applied it on
latest dpdk tree.
Then, tried to apply my vhost PMD patch on it.

When I check the patch, it seems I've faced this issue.
Here are steps to reproduce.

1. Start vhost-user backend application.
     (In my case, testpmd using vhost PMD is the application)
2. Start a VM with vhost-user.
     You can see below message from the backend application.
      VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
      VHOST_CONFIG: set queue enable: 1 to qp idx: 0
      (snip)
      VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
3. After booting Linux on guest, bind the virtio-net device to igb_uio.
    Then below messages are shown.
    VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
    VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
    VHOST_CONFIG: read message VHOST_USER_GET_VRING_BASE

The point is we will have VHOST_USER_RESET_OWNER before
VHOST_USER_GET_VRING_BASE.
Currently, in RESET_OWNER function, all virtio-net data is initialized.
As a result, we also initialize virtio-net flags.
When we get GET_VRING_BASE, we cannot call destroy callback handler
because RUNNING flag has been initialized already.

 I guess when we get RESET_OWNER message, I don't need to do anything.
And all finalizations should be done in GET_VRING_BASE.
(Or some finalizations might be done when next SET_MEM_TABLE is called.)

Thanks,
Tetsuya

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

[Yuanhan Liu]

On Mon, Oct 26, 2015 at 02:54:07PM +0900, Tetsuya Mukawa wrote:
> On 2015/10/25 2:16, Thomas Monjalon wrote:
> > 2015-08-12 03:34, Xie, Huawei:
> >> On 8/8/2015 1:21 AM, Jan Kiszka wrote:
> >>> 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.
> > [...]
> >>> --- 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);
> >> To me this patch makes sense here.
> >> Whether RESET_OWNER is really needed is another question. Whenever the
> >> vhost itself needs to process the vhost device, we need to notify the
> >> switch application to remove it from data plane.
> > Huawei,
> > some patches have been accepted for RESET_OWNER management.
> > Is this patch obsolete?

I think it's still appliable, at least so far.

> 
> Hi Yuanhan and Huawei,
> 
> I also have the same question. Do we have a patch for this issue?
> 
> Today, I've download Yuanhan's multiple queues patches and applied it on
> latest dpdk tree.
> Then, tried to apply my vhost PMD patch on it.
> 
> When I check the patch, it seems I've faced this issue.
> Here are steps to reproduce.

Above patch should fix your issue, right? If so, we need it.

> 
> 1. Start vhost-user backend application.
>      (In my case, testpmd using vhost PMD is the application)
> 2. Start a VM with vhost-user.
>      You can see below message from the backend application.
>       VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
>       VHOST_CONFIG: set queue enable: 1 to qp idx: 0
>       (snip)
>       VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> 3. After booting Linux on guest, bind the virtio-net device to igb_uio.
>     Then below messages are shown.
>     VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>     VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>     VHOST_CONFIG: read message VHOST_USER_GET_VRING_BASE
> 
> The point is we will have VHOST_USER_RESET_OWNER before
> VHOST_USER_GET_VRING_BASE.

Note that there is an ongoing work at QEMU community (from me) to
handle RESET_OWNER correctly: it will be moved to somewhere else
instead of before VHOST_USER_GET_VRING_BASE.

	--yliu

> Currently, in RESET_OWNER function, all virtio-net data is initialized.
> As a result, we also initialize virtio-net flags.
> When we get GET_VRING_BASE, we cannot call destroy callback handler
> because RUNNING flag has been initialized already.
> 
>  I guess when we get RESET_OWNER message, I don't need to do anything.
> And all finalizations should be done in GET_VRING_BASE.
> (Or some finalizations might be done when next SET_MEM_TABLE is called.)
> 
> Thanks,
> Tetsuya

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

[Tetsuya Mukawa]

On 2015/10/26 15:30, Yuanhan Liu wrote:
> On Mon, Oct 26, 2015 at 02:54:07PM +0900, Tetsuya Mukawa wrote:
>> On 2015/10/25 2:16, Thomas Monjalon wrote:
>>> 2015-08-12 03:34, Xie, Huawei:
>>>> On 8/8/2015 1:21 AM, Jan Kiszka wrote:
>>>>> 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.
>>> [...]
>>>>> --- 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);
>>>> To me this patch makes sense here.
>>>> Whether RESET_OWNER is really needed is another question. Whenever the
>>>> vhost itself needs to process the vhost device, we need to notify the
>>>> switch application to remove it from data plane.
>>> Huawei,
>>> some patches have been accepted for RESET_OWNER management.
>>> Is this patch obsolete?
> I think it's still appliable, at least so far.
>
>> Hi Yuanhan and Huawei,
>>
>> I also have the same question. Do we have a patch for this issue?
>>
>> Today, I've download Yuanhan's multiple queues patches and applied it on
>> latest dpdk tree.
>> Then, tried to apply my vhost PMD patch on it.
>>
>> When I check the patch, it seems I've faced this issue.
>> Here are steps to reproduce.
> Above patch should fix your issue, right? If so, we need it.

Yes, the patch will fix the issue.

>> 1. Start vhost-user backend application.
>>      (In my case, testpmd using vhost PMD is the application)
>> 2. Start a VM with vhost-user.
>>      You can see below message from the backend application.
>>       VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
>>       VHOST_CONFIG: set queue enable: 1 to qp idx: 0
>>       (snip)
>>       VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
>> 3. After booting Linux on guest, bind the virtio-net device to igb_uio.
>>     Then below messages are shown.
>>     VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>>     VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>>     VHOST_CONFIG: read message VHOST_USER_GET_VRING_BASE
>>
>> The point is we will have VHOST_USER_RESET_OWNER before
>> VHOST_USER_GET_VRING_BASE.
> Note that there is an ongoing work at QEMU community (from me) to
> handle RESET_OWNER correctly: it will be moved to somewhere else
> instead of before VHOST_USER_GET_VRING_BASE.
>
> 	--yliu

Sounds great! Thanks for handling it.

Tetsuya

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

[Thomas Monjalon]

2015-08-07 19:20, Jan Kiszka:
> 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.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

For an unknown reason, this patch has been missed and
another one replaced it in DPDK 2.2:
http://dpdk.org/browse/dpdk/commit/?id=d243ecf0

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

[Jan Kiszka]

On 2016-03-17 15:42, Thomas Monjalon wrote:
> 2015-08-07 19:20, Jan Kiszka:
>> 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.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> For an unknown reason, this patch has been missed and
> another one replaced it in DPDK 2.2:
> http://dpdk.org/browse/dpdk/commit/?id=d243ecf0
> 

But the bug is fixed now - that is what matters :)

Thanks,
Jan

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