Re: [Xen-devel] [PATCH] xend, sparse: move vcpu-hotplug to xenbus/store

[Ryan Harper <ryanh@xxxxxxxxxx>]

* Rusty Russell <rusty@xxxxxxxxxxxxxxx> [2005-08-08 08:58]:
> On Fri, 2005-08-05 at 14:21 -0500, Ryan Harper wrote:
> > +/* xenbus watch struct */
> > +static struct xenbus_watch cpus_watch = {
> > +   .node = "cpus",
> > +   .callback = handle_cpus_watch,
> > +};
> 
> Everywhere else we've used singular
> names: /domain, /domain/<uuid>/device, /tool, /domain/<uuid>/backend.
> So I'd suggest either using "cpu" or consider putting this under
> "device" and actually using the xenbus system, although I'm not sure
> that's all that useful for you.  Perhaps revisit after Christian has
> done latest merge.

OK.

> 
> > +static void handle_cpus_watch(struct xenbus_watch *watch, const char *node)
> 
> Just a preference, I prefer the watch callback to be something like
> "handle_XXX_event", which seems clearer to me.
> 

Sure.

> > +   /* get a pointer to start of cpus/cpu string */
> > +   if ((cpustr = strstr(node, "cpus/cpu")) != NULL) {
> > +
> > +           /* find which cpu state changed, note vcpu for handler */
> > +           sscanf(cpustr, "cpus/cpu%d", &cpu);
> 
> Please don't repeat "cpu", just use "cpu/0", "cpu/1" which matches what
> block interfaces are doing.
> 
> The guidelines I've been working on with store layout are as follows (no
> doubt we'll all come up with more as we use this thing):
> 
> 1) Redundant information in the store is bad.  Don't put information in
> two places then insist they be the same.
> 
> 2) Multiple entries should be in directory names, unless the hierarchy
> is getting completely out of control.
> 
> 3) Directory names have to be unique, obviously.  If there is a useful
> handle to use, do so, otherwise simply use numbers starting from 0,
> which makes it fairly clear to the reader the tags are arbitrary.
> 
> I'm still not sure about binary flags.  For the block devices, Christian
> and I chose "read-only": if it exists, the device is read only, if not,
> it's read-write.  That's preferable to a "writability" flag which
> contains either "read-only" or "read-write", since the default is
> fairly-clear.

Good to know.

> 
> In the case of online vs. offline cpus, you could either have an
> "offline" entry (not existing meaning "online", which is probably the
> clearer default), or a "availability" which contains the string "online"
> or "offline".  Please do not have a node called "online" which contains
> 0 to mean "NOT"!

In this case, I was just following (with the exception of cpus) the
sysfs example which already exists:

/sys/devices/system/cpu/cpu0/online 


If we don't care that the xenbus entries _do_ the exact same thing, but
aren't similarly named, I'm fine with that.

Just to be clear, it sounds like you want:

/domain/uuid/cpu/0/online  # cpu 0 is online
/domain/uuid/cpu/1/offline # cpu 1 is offline
/domain/uuid/cpu/2/online  # etc.
/domain/uuid/cpu/3/online  # etc.

I don't like that.  What about this?

/domain/uuid/cpu/0/state  

and the values can be "online|offline"

I rather like having a single target whose value changes rather than
two entries one of which I must remove when the state changes.  In this
case, I can keep a single entry but toss the online=0 stuff you
dislike.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@xxxxxxxxxx

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