Xen devel - May 2013 - [PATCH V3 0/2] xenbus: Fix S3 frontend resume when xenstored is not running

Hi,

This patch series fixes the S3 resume of a domain running xenstored and a
frontend over xenbus (xen-netfront in my use case).

As device resume is happening before process resume, the xenbus frontend
resume is hanging if xenstored is not running, thus causing a deadlock.
This patch series is fixing that issue by deferring the xenbus frontend
resume when we are running xenstored in that same domain.

Changes in v2:
- Instead of bypassing the resume, process it in a workqueue.
Changes in v3:
- Add a struct work in xenbus_device to avoid dynamic allocation
- Use Linux global workqueue
- Several small code fixes

Aurelien Chartier (2):
  xenbus: save xenstore local status for later use
  xenbus: delay xenbus frontend resume is xenstored is not running

 drivers/xen/xenbus/xenbus_comms.h          |    1 +
 drivers/xen/xenbus/xenbus_probe.c          |   27 ++++++++++++---------------
 drivers/xen/xenbus/xenbus_probe.h          |    7 +++++++
 drivers/xen/xenbus/xenbus_probe_frontend.c |   26 +++++++++++++++++++++++++-
 include/xen/xenbus.h                       |    1 +
 5 files changed, 46 insertions(+), 16 deletions(-)

-- 
1.7.10.4

Save the xenstore local status computed in xenbus_init. It can then be used
later to check if xenstored is running in that domain

Signed-off-by: Aurelien Chartier <aurelien.chartier@citrix.com>
---
 drivers/xen/xenbus/xenbus_comms.h |    1 +
 drivers/xen/xenbus/xenbus_probe.c |   27 ++++++++++++---------------
 drivers/xen/xenbus/xenbus_probe.h |    7 +++++++
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_comms.h
b/drivers/xen/xenbus/xenbus_comms.h
index c8abd3b..be329a1 100644
--- a/drivers/xen/xenbus/xenbus_comms.h
+++ b/drivers/xen/xenbus/xenbus_comms.h
@@ -45,6 +45,7 @@ int xb_wait_for_data_to_read(void);
 int xs_input_avail(void);
 extern struct xenstore_domain_interface *xen_store_interface;
 extern int xen_store_evtchn;
+extern enum xenstore_init xen_store_domain;
 
 extern const struct file_operations xen_xenbus_fops;
 
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index 3325884..c3375ba 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -69,6 +69,9 @@ EXPORT_SYMBOL_GPL(xen_store_evtchn);
 struct xenstore_domain_interface *xen_store_interface;
 EXPORT_SYMBOL_GPL(xen_store_interface);
 
+enum xenstore_init xen_store_domain;
+EXPORT_SYMBOL_GPL(xen_store_domain);
+
 static unsigned long xen_store_mfn;
 
 static BLOCKING_NOTIFIER_HEAD(xenstore_chain);
@@ -719,17 +722,11 @@ static int __init xenstored_local_init(void)
 	return err;
 }
 
-enum xenstore_init {
-	UNKNOWN,
-	PV,
-	HVM,
-	LOCAL,
-};
 static int __init xenbus_init(void)
 {
 	int err = 0;
-	enum xenstore_init usage = UNKNOWN;
 	uint64_t v = 0;
+	xen_store_domain = XS_UNKNOWN;
 
 	if (!xen_domain())
 		return -ENODEV;
@@ -737,29 +734,29 @@ static int __init xenbus_init(void)
 	xenbus_ring_ops_init();
 
 	if (xen_pv_domain())
-		usage = PV;
+		xen_store_domain = XS_PV;
 	if (xen_hvm_domain())
-		usage = HVM;
+		xen_store_domain = XS_HVM;
 	if (xen_hvm_domain() && xen_initial_domain())
-		usage = LOCAL;
+		xen_store_domain = XS_LOCAL;
 	if (xen_pv_domain() && !xen_start_info->store_evtchn)
-		usage = LOCAL;
+		xen_store_domain = XS_LOCAL;
 	if (xen_pv_domain() && xen_start_info->store_evtchn)
 		xenstored_ready = 1;
 
-	switch (usage) {
-	case LOCAL:
+	switch (xen_store_domain) {
+	case XS_LOCAL:
 		err = xenstored_local_init();
 		if (err)
 			goto out_error;
 		xen_store_interface = mfn_to_virt(xen_store_mfn);
 		break;
-	case PV:
+	case XS_PV:
 		xen_store_evtchn = xen_start_info->store_evtchn;
 		xen_store_mfn = xen_start_info->store_mfn;
 		xen_store_interface = mfn_to_virt(xen_store_mfn);
 		break;
-	case HVM:
+	case XS_HVM:
 		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
 		if (err)
 			goto out_error;
diff --git a/drivers/xen/xenbus/xenbus_probe.h
b/drivers/xen/xenbus/xenbus_probe.h
index bb4f92e..146f857 100644
--- a/drivers/xen/xenbus/xenbus_probe.h
+++ b/drivers/xen/xenbus/xenbus_probe.h
@@ -47,6 +47,13 @@ struct xen_bus_type {
 	struct bus_type bus;
 };
 
+enum xenstore_init {
+	XS_UNKNOWN,
+	XS_PV,
+	XS_HVM,
+	XS_LOCAL,
+};
+
 extern struct device_attribute xenbus_dev_attrs[];
 
 extern int xenbus_match(struct device *_dev, struct device_driver *_drv);
-- 
1.7.10.4

If the xenbus frontend is located in a domain running xenstored, the device
resume is hanging because it is happening before the process resume. This
patch adds extra logic to the resume code to check if we are the domain
running xenstored and delay the resume if needed.

Signed-off-by: Aurelien Chartier <aurelien.chartier@citrix.com>
---
 drivers/xen/xenbus/xenbus_probe_frontend.c |   26 +++++++++++++++++++++++++-
 include/xen/xenbus.h                       |    1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 3159a37..217b9c1 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -89,9 +89,33 @@ static void backend_changed(struct xenbus_watch *watch,
 	xenbus_otherend_changed(watch, vec, len, 1);
 }
 
+static void xenbus_frontend_delayed_resume(struct work_struct *w)
+{
+	struct xenbus_device *xdev = container_of(w, struct xenbus_device, work);
+
+	xenbus_dev_resume(&xdev->dev);
+}
+
+static int xenbus_frontend_dev_resume(struct device *dev)
+{
+	/* 
+	 * If xenstored is running in that domain, we cannot access the backend
+	 * state at the moment, so we need to defer xenbus_dev_resume
+	 */
+	if (xen_store_domain == XS_LOCAL) {
+		struct xenbus_device *xdev = to_xenbus_device(dev);
+
+		INIT_WORK(&xdev->work, xenbus_frontend_delayed_resume);
+		schedule_work(&xdev->work);
+
+		return 0;
+	}
+
+	return xenbus_dev_resume(dev);
+}
 static const struct dev_pm_ops xenbus_pm_ops = {
 	.suspend	= xenbus_dev_suspend,
-	.resume		= xenbus_dev_resume,
+	.resume		= xenbus_frontend_dev_resume,
 	.freeze		= xenbus_dev_suspend,
 	.thaw		= xenbus_dev_cancel,
 	.restore	= xenbus_dev_resume,
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 0a7515c..569c07f 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -70,6 +70,7 @@ struct xenbus_device {
 	struct device dev;
 	enum xenbus_state state;
 	struct completion down;
+	struct work_struct work;
 };
 
 static inline struct xenbus_device *to_xenbus_device(struct device *dev)
-- 
1.7.10.4

On 13/05/13 14:13, Aurelien Chartier wrote:
> If the xenbus frontend is located in a domain running xenstored, the device
> resume is hanging because it is happening before the process resume. This
> patch adds extra logic to the resume code to check if we are the domain
> running xenstored and delay the resume if needed.
Would this be better done at the bus level instead of per-device?
> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> @@ -89,9 +89,33 @@ static void backend_changed(struct xenbus_watch *watch,
>  	xenbus_otherend_changed(watch, vec, len, 1);
>  }
>  
> +static void xenbus_frontend_delayed_resume(struct work_struct *w)
> +{
> +	struct xenbus_device *xdev = container_of(w, struct xenbus_device, work);
> +
> +	xenbus_dev_resume(&xdev->dev);
> +}
> +
> +static int xenbus_frontend_dev_resume(struct device *dev)
> +{
> +	/* 
> +	 * If xenstored is running in that domain, we cannot access the backend
> +	 * state at the moment, so we need to defer xenbus_dev_resume
> +	 */
I think you mean "If xenstored is running in /this/ domain, we cannot
access the backend state at the moment..."
> +	if (xen_store_domain == XS_LOCAL) {
> +		struct xenbus_device *xdev = to_xenbus_device(dev);
> +
> +		INIT_WORK(&xdev->work, xenbus_frontend_delayed_resume);
> +		schedule_work(&xdev->work);
How does this ensure xenstored is running when the work is scheduled?
Will it end up blocking on a system workqueue until xenstored is
running?  That would be bad.

David

On 13/05/13 14:13, Aurelien Chartier wrote:
> Save the xenstore local status computed in xenbus_init. It can then be used
> later to check if xenstored is running in that domain
> 
> Signed-off-by: Aurelien Chartier <aurelien.chartier@citrix.com>
Looks straightforward to me.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/xen/xenbus/xenbus_comms.h |    1 +
>  drivers/xen/xenbus/xenbus_probe.c |   27 ++++++++++++---------------
>  drivers/xen/xenbus/xenbus_probe.h |    7 +++++++
>  3 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_comms.h
b/drivers/xen/xenbus/xenbus_comms.h
> index c8abd3b..be329a1 100644
> --- a/drivers/xen/xenbus/xenbus_comms.h
> +++ b/drivers/xen/xenbus/xenbus_comms.h
> @@ -45,6 +45,7 @@ int xb_wait_for_data_to_read(void);
>  int xs_input_avail(void);
>  extern struct xenstore_domain_interface *xen_store_interface;
>  extern int xen_store_evtchn;
> +extern enum xenstore_init xen_store_domain;
This name suggests it''s used for a dom ID.  Perhaps
xen_store_domain_type?

David

On 14/05/13 11:09, David Vrabel wrote:
> On 13/05/13 14:13, Aurelien Chartier wrote:
>> If the xenbus frontend is located in a domain running xenstored, the
device
>> resume is hanging because it is happening before the process resume.
This
>> patch adds extra logic to the resume code to check if we are the domain
>> running xenstored and delay the resume if needed.
> Would this be better done at the bus level instead of per-device?
Linux driver resume code is calling the resume function defined in the
dev_pm_ops field of the bus. I don''t see any way to have that resume
done at the bus level for xenbus, but I may have missed something ?
>
>> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
>> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
>> @@ -89,9 +89,33 @@ static void backend_changed(struct xenbus_watch
*watch,
>>  	xenbus_otherend_changed(watch, vec, len, 1);
>>  }
>>  
>> +static void xenbus_frontend_delayed_resume(struct work_struct *w)
>> +{
>> +	struct xenbus_device *xdev = container_of(w, struct xenbus_device,
work);
>> +
>> +	xenbus_dev_resume(&xdev->dev);
>> +}
>> +
>> +static int xenbus_frontend_dev_resume(struct device *dev)
>> +{
>> +	/* 
>> +	 * If xenstored is running in that domain, we cannot access the
backend
>> +	 * state at the moment, so we need to defer xenbus_dev_resume
>> +	 */
> I think you mean "If xenstored is running in /this/ domain, we cannot
> access the backend state at the moment..."
Right. I will fix it.
>
>> +	if (xen_store_domain == XS_LOCAL) {
>> +		struct xenbus_device *xdev = to_xenbus_device(dev);
>> +
>> +		INIT_WORK(&xdev->work, xenbus_frontend_delayed_resume);
>> +		schedule_work(&xdev->work);
> How does this ensure xenstored is running when the work is scheduled?
> Will it end up blocking on a system workqueue until xenstored is
> running?  That would be bad.
Yes, that will be blocked until xenstored process has been resumed,
since it is the current behavior. However, as the process resume is now
happening in parallel, it will only stay blocked for a very short period
of time.

On 14/05/13 11:17, David Vrabel wrote:
> On 13/05/13 14:13, Aurelien Chartier wrote:
>> Save the xenstore local status computed in xenbus_init. It can then be
used
>> later to check if xenstored is running in that domain
>>
>> Signed-off-by: Aurelien Chartier <aurelien.chartier@citrix.com>
> Looks straightforward to me.
>
> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
>
>> ---
>>  drivers/xen/xenbus/xenbus_comms.h |    1 +
>>  drivers/xen/xenbus/xenbus_probe.c |   27 ++++++++++++---------------
>>  drivers/xen/xenbus/xenbus_probe.h |    7 +++++++
>>  3 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_comms.h
b/drivers/xen/xenbus/xenbus_comms.h
>> index c8abd3b..be329a1 100644
>> --- a/drivers/xen/xenbus/xenbus_comms.h
>> +++ b/drivers/xen/xenbus/xenbus_comms.h
>> @@ -45,6 +45,7 @@ int xb_wait_for_data_to_read(void);
>>  int xs_input_avail(void);
>>  extern struct xenstore_domain_interface *xen_store_interface;
>>  extern int xen_store_evtchn;
>> +extern enum xenstore_init xen_store_domain;
> This name suggests it''s used for a dom ID.  Perhaps
xen_store_domain_type?
Indeed, the name is confusing. I will change it for xen_store_domain_type.

On 14/05/13 14:04, Aurelien Chartier wrote:
> On 14/05/13 11:09, David Vrabel wrote:
>> On 13/05/13 14:13, Aurelien Chartier wrote:
>>> If the xenbus frontend is located in a domain running xenstored,
the device
>>> resume is hanging because it is happening before the process
resume. This
>>> patch adds extra logic to the resume code to check if we are the
domain
>>> running xenstored and delay the resume if needed.
>> Would this be better done at the bus level instead of per-device?
> Linux driver resume code is calling the resume function defined in the
> dev_pm_ops field of the bus. I don''t see any way to have that
resume
> done at the bus level for xenbus, but I may have missed something ?
No, I was misremembering.
>>> +	if (xen_store_domain == XS_LOCAL) {
>>> +		struct xenbus_device *xdev = to_xenbus_device(dev);
>>> +
>>> +		INIT_WORK(&xdev->work, xenbus_frontend_delayed_resume);
>>> +		schedule_work(&xdev->work);
>> How does this ensure xenstored is running when the work is scheduled?
>> Will it end up blocking on a system workqueue until xenstored is
>> running?  That would be bad.
> 
> Yes, that will be blocked until xenstored process has been resumed,
> since it is the current behavior. However, as the process resume is now
> happening in parallel, it will only stay blocked for a very short period
> of time.
This may cause deadlocks.

It used to be that a work item could block the execution of other work
on that workqueue by sleeping or not completing,  but workqueues have
changed quite a bit since I last looked at them detail so this might not
be the case any more.

As resuming N devices queues N work items this can cause N worker
threads to block.  Is it guaranteed that the kernel can continue to
create more workers to handle any work items required before xenstored
can start?

The safe and obviously correct method would be to create a dedicated
workqueue for this resume work.

David

On 14/05/13 14:38, David Vrabel wrote:
>
>>>> +	if (xen_store_domain == XS_LOCAL) {
>>>> +		struct xenbus_device *xdev = to_xenbus_device(dev);
>>>> +
>>>> +		INIT_WORK(&xdev->work,
xenbus_frontend_delayed_resume);
>>>> +		schedule_work(&xdev->work);
>>> How does this ensure xenstored is running when the work is
scheduled?
>>> Will it end up blocking on a system workqueue until xenstored is
>>> running?  That would be bad.
>> Yes, that will be blocked until xenstored process has been resumed,
>> since it is the current behavior. However, as the process resume is now
>> happening in parallel, it will only stay blocked for a very short
period
>> of time.
> This may cause deadlocks.
>
> It used to be that a work item could block the execution of other work
> on that workqueue by sleeping or not completing,  but workqueues have
> changed quite a bit since I last looked at them detail so this might not
> be the case any more.
>
> As resuming N devices queues N work items this can cause N worker
> threads to block.  Is it guaranteed that the kernel can continue to
> create more workers to handle any work items required before xenstored
> can start?
>
> The safe and obviously correct method would be to create a dedicated
> workqueue for this resume work.
Ok, I get your point. I used a dedicated workqueue in my previous patch
but switched to use schedule_work, since I thought it was not worth
having a dedicated one. I''ll switch back to that in the v4.

Thanks,
Aurelien