Xen devel - Aug 2005 - [PATCH] Convert shutdown to use xenstore

The attached patch:

1. Converts the shutdown driver and xend to use the store instead of
   control messages, 

2. Includes Anthony''s xenstore notification code, and

3. Changes xend so that sysrq''s are no longer sent as "special
case"
   shutdown messages.  Store keys are cheap, so making the sysrq
   delivery less obscure is good.

I think I have made all of the appropriate modifications to Xend, but
it is complex, so I may have missed something.  Comments are welcome.

Signed-off-by: Dan Smith <danms@us.ibm.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>






-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms@us.ibm.com

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi Dan,

Any reason not to use textual data in the store rather than numbers?  IMO
it''d
make it a little more readable - numbers only make sense if you have the 
header file - but I don''t know if there''s a convention on
this.

Cheers,
Mark

On Thursday 04 August 2005 16:35, Dan Smith wrote:
> The attached patch:
>
> 1. Converts the shutdown driver and xend to use the store instead of
>    control messages,
>
> 2. Includes Anthony''s xenstore notification code, and
>
> 3. Changes xend so that sysrq''s are no longer sent as
"special case"
>    shutdown messages.  Store keys are cheap, so making the sysrq
>    delivery less obscure is good.
>
> I think I have made all of the appropriate modifications to Xend, but
> it is complex, so I may have missed something.  Comments are welcome.
>
> Signed-off-by: Dan Smith <danms@us.ibm.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2005-08-04 at 08:35 -0700, Dan Smith wrote:
> 2. Includes Anthony''s xenstore notification code, and
There are subtleties here.  We do need a notifier, because in domain 0,
the store gets started long after boot (when xenstored starts).
However, we don''t need a notifier for domU code, because the store
exists from the moment we boot.

ie. for domU, this notifier chain is a noop, and I think the name should
reflect that, otherwise we''ll have innocent drivers thinking
registering
is the right thing.  register_dom0_xenstore_start()?

Or we could hide the whole thing, and simply call the notifier
immediately in the dom0/store-already-up case:

/* Only required if your code runs in dom0 as well as domU */
static int register_xenstore_notifier(struct notifier_block *notifier)
{
	int ret = 0;

	down(&xenbus_lock);
	if (xen_start_info.evtchn)
		ret = notifier.notifier_call(notifier, 0, NULL);
	else
		notifier_chain_register(&xenstore_chain, nb);
	up(&xenbus_lock);
	return ret;
}

Implementation detail: just use the xenbus_lock rather than another
lock, and make sure you hold it while traversing, not just registering!

Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Rusty Russell wrote:
>/* Only required if your code runs in dom0 as well as domU */
>static int register_xenstore_notifier(struct notifier_block *notifier)
>{
>	int ret = 0;
>
>	down(&xenbus_lock);
>	if (xen_start_info.evtchn)
>		ret = notifier.notifier_call(notifier, 0, NULL);
>	else
>		notifier_chain_register(&xenstore_chain, nb);
>	up(&xenbus_lock);
>	return ret;
>}
>  
>
I like this.  It avoids having special case code for dom0 in all the 
drivers.
>Implementation detail: just use the xenbus_lock rather than another
>lock, and make sure you hold it while traversing, not just registering!
>  
>
I was following kernel/cpu.c:cpu_down().  Definitely makes sense though.

Regards,

Anthony Liguori
>Rusty.
>  
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2005-08-05 at 02:58 +0100, Mark Williamson
wrote:
> Hi Dan,
> 
> Any reason not to use textual data in the store rather than numbers?  IMO
it''d
> make it a little more readable - numbers only make sense if you have the 
> header file - but I don''t know if there''s a convention on
this.
If something fundamentally *is* a number, or an ordinal, that''s one
thing.  But otherwise, a simple value is better.

We''ve seen this in sysfs and Open Firmware; the only defence against
crap accumulating in the store is constant vigilance.  This is only
likely if it''s actually useful to browse the thing, ie. as consistent
and human-readable as possible.  This is also an end in itself:
something that is self-evident and easy to understand is easier to
maintain ("hey, I was using the lower bit of that value to determine if
it''s read-only, and you broke it!" vs "you removed the field
called
"read-only").

An ideal system is one where I can implement a new backend, frontend or
tool simply by looking at the paths and values in the store.  It also
has clear implications for diagnosing problems.

Cheers,
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 8/5/05, Dan Smith <danms@us.ibm.com> wrote:
> The attached patch:
> 
> 1. Converts the shutdown driver and xend to use the store instead of
>    control messages,
> 
> 2. Includes Anthony''s xenstore notification code, and
> 
> 3. Changes xend so that sysrq''s are no longer sent as
"special case"
>    shutdown messages.  Store keys are cheap, so making the sysrq
>    delivery less obscure is good.
> 
+static int setup_shutdown_watcher(struct notifier_block *notifier,
+                                  unsigned long event,
+                                  void *data)
+{
+    int err1=0, err2=0;
+
+    down(&xenbus_lock);
+    err1 = register_xenbus_watch(&shutdown_watch);
+#ifdef CONFIG_MAGIC_SYSRQ
+    err2 = register_xenbus_watch(&sysrq_watch);
+#endif
+    up(&xenbus_lock);
+
+    if (err1) {
+        printk("Failed to set shutdown watcher\n");
+    }

How about putting declaration of err2 inside a "#ifdef
CONFIG_MAGIC_SYSRQ" ?

Besides i think some printk should use KERN_ERR to report error properly.

regards,
aq

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

aq> How about putting declaration of err2 inside a "#ifdef
aq> CONFIG_MAGIC_SYSRQ" ?

Oh, I guess that''ll issue a warning if CONFIG_MAGIC_SYSRQ
isn''t
enabled, won''t it?

aq> Besides i think some printk should use KERN_ERR to report error
aq> properly.

OK.  I''ll wrap these and the other suggestions in a new patch and
re-submit tomorrow.

-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms@us.ibm.com


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 5 Aug 2005, at 03:09, Rusty Russell wrote:
> Or we could hide the whole thing, and simply call the notifier
> immediately in the dom0/store-already-up case:
Yes!

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel