[PATCH] xen: fix frontend driver disconnected from xenbus on removal

[PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Hi, all!

While working on DRM PV frontend driver I faced an issue with
driver removal, e.g. when driver's .remove callback is called
the driver is already disconnected form the xenbus and it is not
possible to synchronize the process of removal with the backend.
Backend in my case is a user-space application, so this may explain
why backends which are kernel modules do not suffer from this:
it seems that user-space is clumsy and its states are not delivered
through xenbus fast enough.

The obvious way to fix this behavior is to disconnect frontend
drivers from xenbus _after_ their .remove callback is called.

Thank you,
Oleksandr Andrushchenko

Oleksandr Andrushchenko (1):
  xen: fix frontend driver disconnected from xenbus on removal

 drivers/xen/xenbus/xenbus_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Current xenbus frontend driver removal flow first disconnects
the driver from xenbus and then calls driver's remove callback.
This makes it impossible for the driver to listen to backend's
state changes and synchronize the removal procedure.

Fix this by removing other end XenBus watches after the
driver's remove callback is called.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 drivers/xen/xenbus/xenbus_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 74888cacd0b0..9c63cd3f416b 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -258,11 +258,11 @@ int xenbus_dev_remove(struct device *_dev)
 
 	DPRINTK("%s", dev->nodename);
 
-	free_otherend_watch(dev);
-
 	if (drv->remove)
 		drv->remove(dev);
 
+	free_otherend_watch(dev);
+
 	free_otherend_details(dev);
 
 	xenbus_switch_state(dev, XenbusStateClosed);
-- 
2.7.4

Re: [PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Boris Ostrovsky]

On 02/01/2018 03:57 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Current xenbus frontend driver removal flow first disconnects
> the driver from xenbus and then calls driver's remove callback.
> This makes it impossible for the driver to listen to backend's
> state changes and synchronize the removal procedure.
>
> Fix this by removing other end XenBus watches after the
> driver's remove callback is called.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>  drivers/xen/xenbus/xenbus_probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index 74888cacd0b0..9c63cd3f416b 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -258,11 +258,11 @@ int xenbus_dev_remove(struct device *_dev)
>  
>  	DPRINTK("%s", dev->nodename);
>  
> -	free_otherend_watch(dev);
> -
>  	if (drv->remove)
>  		drv->remove(dev);


Is it possible for the watch to fire here?

-boris

>  
> +	free_otherend_watch(dev);
> +
>  	free_otherend_details(dev);
>  
>  	xenbus_switch_state(dev, XenbusStateClosed);

Re: [PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Oleksandr Andrushchenko]

On 02/01/2018 10:08 PM, Boris Ostrovsky wrote:
> On 02/01/2018 03:57 AM, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Current xenbus frontend driver removal flow first disconnects
>> the driver from xenbus and then calls driver's remove callback.
>> This makes it impossible for the driver to listen to backend's
>> state changes and synchronize the removal procedure.
>>
>> Fix this by removing other end XenBus watches after the
>> driver's remove callback is called.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> ---
>>   drivers/xen/xenbus/xenbus_probe.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
>> index 74888cacd0b0..9c63cd3f416b 100644
>> --- a/drivers/xen/xenbus/xenbus_probe.c
>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>> @@ -258,11 +258,11 @@ int xenbus_dev_remove(struct device *_dev)
>>   
>>   	DPRINTK("%s", dev->nodename);
>>   
>> -	free_otherend_watch(dev);
>> -
>>   	if (drv->remove)
>>   		drv->remove(dev);
>
> Is it possible for the watch to fire here?
Indeed. Yes, It is possible, so we have to somehow protect the removed
driver from being called, e.g. the driver cleans up in its .remove,
but watch may still trigger .otherend_changed callback.
Is this what you mean?
If so, do you have something neat on your mind how to solve this?
> -boris
>
>>   
>> +	free_otherend_watch(dev);
>> +
>>   	free_otherend_details(dev);
>>   
>>   	xenbus_switch_state(dev, XenbusStateClosed);
Thank you,
Oleksandr

Re: [PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Boris Ostrovsky]

On 02/01/2018 03:24 PM, Oleksandr Andrushchenko wrote:
>
>
> On 02/01/2018 10:08 PM, Boris Ostrovsky wrote:
>> On 02/01/2018 03:57 AM, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> Current xenbus frontend driver removal flow first disconnects
>>> the driver from xenbus and then calls driver's remove callback.
>>> This makes it impossible for the driver to listen to backend's
>>> state changes and synchronize the removal procedure.
>>>
>>> Fix this by removing other end XenBus watches after the
>>> driver's remove callback is called.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <oleksandr_andrushchenko@epam.com>
>>> ---
>>>   drivers/xen/xenbus/xenbus_probe.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c
>>> b/drivers/xen/xenbus/xenbus_probe.c
>>> index 74888cacd0b0..9c63cd3f416b 100644
>>> --- a/drivers/xen/xenbus/xenbus_probe.c
>>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>>> @@ -258,11 +258,11 @@ int xenbus_dev_remove(struct device *_dev)
>>>         DPRINTK("%s", dev->nodename);
>>>   -    free_otherend_watch(dev);
>>> -
>>>       if (drv->remove)
>>>           drv->remove(dev);
>>
>> Is it possible for the watch to fire here?
> Indeed. Yes, It is possible, so we have to somehow protect the removed
> driver from being called, e.g. the driver cleans up in its .remove,
> but watch may still trigger .otherend_changed callback.
> Is this what you mean?

(-David who is not at Citrix anymore)

Exactly.

That's why otherend cleanup is split into free_otherend_watch() and
free_otherend_details().


> If so, do you have something neat on your mind how to solve this?

Not necessarily "neat" but perhaps you can use
xenbus_read_otherend_details() in both front and back ends. After all,
IIUIC you are doing something synchronously so you don't really need a
watch.

-boris

>
>> -boris
>>
>>>   +    free_otherend_watch(dev);
>>> +
>>>       free_otherend_details(dev);
>>>         xenbus_switch_state(dev, XenbusStateClosed);
> Thank you,
> Oleksandr

Re: [PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Oleksandr Andrushchenko]

On 02/01/2018 11:09 PM, Boris Ostrovsky wrote:
> On 02/01/2018 03:24 PM, Oleksandr Andrushchenko wrote:
>>
>> On 02/01/2018 10:08 PM, Boris Ostrovsky wrote:
>>> On 02/01/2018 03:57 AM, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>
>>>> Current xenbus frontend driver removal flow first disconnects
>>>> the driver from xenbus and then calls driver's remove callback.
>>>> This makes it impossible for the driver to listen to backend's
>>>> state changes and synchronize the removal procedure.
>>>>
>>>> Fix this by removing other end XenBus watches after the
>>>> driver's remove callback is called.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko
>>>> <oleksandr_andrushchenko@epam.com>
>>>> ---
>>>>    drivers/xen/xenbus/xenbus_probe.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c
>>>> b/drivers/xen/xenbus/xenbus_probe.c
>>>> index 74888cacd0b0..9c63cd3f416b 100644
>>>> --- a/drivers/xen/xenbus/xenbus_probe.c
>>>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>>>> @@ -258,11 +258,11 @@ int xenbus_dev_remove(struct device *_dev)
>>>>          DPRINTK("%s", dev->nodename);
>>>>    -    free_otherend_watch(dev);
>>>> -
>>>>        if (drv->remove)
>>>>            drv->remove(dev);
>>> Is it possible for the watch to fire here?
>> Indeed. Yes, It is possible, so we have to somehow protect the removed
>> driver from being called, e.g. the driver cleans up in its .remove,
>> but watch may still trigger .otherend_changed callback.
>> Is this what you mean?
> (-David who is not at Citrix anymore)
>
> Exactly.
>
> That's why otherend cleanup is split into free_otherend_watch() and
> free_otherend_details().
Understood, thank you
Confusion came because of the patch [1]: in .remove we wait
for the backend to change its states in .otherend_changed
callback and wake us, but I am not sure how those state changes
may occur if during .remove the driver has already watches
freed. So, this is why I tried to play around with
free_otherend_watch()...
>
>> If so, do you have something neat on your mind how to solve this?
> Not necessarily "neat" but perhaps you can use
> xenbus_read_otherend_details() in both front and back ends. After all,
> IIUIC you are doing something synchronously so you don't really need a
> watch.
Yes, I will implement a dedicated flow in the .remove
instead of relying on .otherend_changed
> -boris
>
>>> -boris
>>>
>>>>    +    free_otherend_watch(dev);
>>>> +
>>>>        free_otherend_details(dev);
>>>>          xenbus_switch_state(dev, XenbusStateClosed);
>> Thank you,
>> Oleksandr
Thank you,
Oleksandr

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/xen-netfront.c?id=5b5971df3bc2775107ddad164018a8a8db633b81