Hi,
This patch makes xenbus and net/blkfront drivers correctly handle
device_shutdown(). Changes:
* new xenbus_strstate() helper function for fancy debug output.
* add xenbus_dev_shutdown() function which switches the devices
into Closing state and waits for them to finish shutdown.
* new helper function xenbus_closing_done(), which handles the
final state transition and informs xenbus_dev_shutdown().
xenbus_closing_done() behaves differently depending on how the closing
was triggered. If it was triggered by the backend the frontend acks the
removal by going to CLosed. If it was triggered via device_shutdown()
the state is set back to "Initializing", to allow a possible new
kernel
instance to reuse the devices (for domU kexec again ;)
cheers,
Gerd
--
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 17/8/06 3:03 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:> xenbus_closing_done() behaves differently depending on how the closing > was triggered. If it was triggered by the backend the frontend acks the > removal by going to CLosed. If it was triggered via device_shutdown() > the state is set back to "Initializing", to allow a possible new kernel > instance to reuse the devices (for domU kexec again ;)I really don''t like that distinction very much (moving to Initialising instead of Closed). The problem is really in the backend, which incorrectly interprets XenbusStateClosed as a request to completely unregister the device. The reason it does this is because the control tools signal that destruction should take place by deleting the frontend directory in xenstore. This results in xenbus_read_driver_state() returning XenbusStateClosed, hence the backend drivers have interpreted that state value as a destruction request from the tools. I''ve gone some way to cleaning this up in changeset 11203, which instead has xenbus_read_driver_state() return XenbusStateUnknown if the frontend directory is missing. You should use this to distinguish the Closed and Unkown states in the backend drivers: this will mean that you won''t need to avoid entering state Closed in the frontend drivers. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 19/8/06 1:25 pm, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote:> I''ve gone some way to cleaning this up in changeset 11203, which instead has > xenbus_read_driver_state() return XenbusStateUnknown if the frontend > directory is missing. You should use this to distinguish the Closed and > Unkown states in the backend drivers: this will mean that you won''t need to > avoid entering state Closed in the frontend drivers.By the way, changing this frontend behaviour may mean you need to tweak your patch to the backend drivers, allowing reconnection, which I already checked in (changeset 11194). I''m not sure though: maybe that patch will still work. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 17/8/06 3:03 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:> + int shutdown_in_progress;Get rid of this field in your next patch. The use in xenbus_client.c should go away because you should unconditionally move to state XenbusStateClosed, fixing up backend drivers as necessary. The use in netfront.c doesn''t make much sense to me. Is it a side effect of needing to move into state XenbusStateInitialised (which will be going away)? I think we should be able to do without it, anyway (maybe a fix to backend drivers will be required). The use in blkfront.c can be replaced by a check of system_state (e.g., (system_state == SYSTEM_RUNNING)) as defined in linux/kernel.h. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,>> + int shutdown_in_progress; > > Get rid of this field in your next patch.Should be easy with your changes, I''ll have a look today.> The use in netfront.c doesn''t make much sense to me. Is it a side effect of > needing to move into state XenbusStateInitialised (which will be going > away)?Yes. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> http://www.suse.de/~kraxel/julika-dora.jpeg _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> I really don''t like that distinction very much (moving to Initialising > instead of Closed). The problem is really in the backend, which incorrectly > interprets XenbusStateClosed as a request to completely unregister the > device. The reason it does this is because the control tools signal that > destruction should take place by deleting the frontend directory in > xenstore. This results in xenbus_read_driver_state() returning > XenbusStateClosed, hence the backend drivers have interpreted that state > value as a destruction request from the tools.Ok, that should work fine for the backend side. But there is another problem on the frontend side: xenbus_probe_node() ignores devices which are not in Initializing state, so it doesn''t reprobe devices which are about to disappear. The next kernel should probe them, but the frontend state is still Closing (or Closed) ... I think that was the initial reason to do the jumpback to Initializing (so the new kexec''ed kernel finds an environment identical to the direct boot). That was also the reason to introduce the shutdown_in_progress flag, so I have some way to avoid re-initializing of the device by the old kernel. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> http://www.suse.de/~kraxel/julika-dora.jpeg _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 21/8/06 3:11 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:> But there is another problem on the frontend side: xenbus_probe_node() > ignores devices which are not in Initializing state, so it doesn''t > reprobe devices which are about to disappear. The next kernel should > probe them, but the frontend state is still Closing (or Closed) ... > > I think that was the initial reason to do the jumpback to Initializing > (so the new kexec''ed kernel finds an environment identical to the direct > boot). That was also the reason to introduce the shutdown_in_progress > flag, so I have some way to avoid re-initializing of the device by the > old kernel.I think the problem here is that the xenbus state machine does not distinguish who is driving a sequence of state changes. There''s no easy way to determine whether a shutdown is triggered by e.g., backend device hotplug or frontend driver removal / shutdown. Moving to state Initialising in the frontend is not a great solution. For example, what happens if a device is hotplug-removed via xm at the same time the guest is kexec''ing? Instead of moving to Closed the guest will move to Initialising and the hotplug request is ''lost'' because the new guest kernel instance will simply reconnect to the still-waiting backend. I think a better solution is for hotplug events to create a new node in the backend directory. Something nice and explicit, like ''deleted''. Backend drivers can choose to device_unregister() only if XenbusStateUnknown or the node ''deleted'' exists. We can check for ''deleted'' at the otherend in xenbus_probe_node() and bail if we see it. Does this sound plausible? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser wrote:> I think the problem here is that the xenbus state machine does not > distinguish who is driving a sequence of state changes.One of the problems, yes. The most tricky one is the handover of the frontend device from one kernel to the next (same issues will come up for some minios+grub based bootloader if someone goes creating one ...). The prerequisites we have are: * We _must not_ break old domUs. * I''d like to be able to boot old domU kernels via kexec, so the post-kexec environment should be identical to what the domain builder creates today. The backward compatibility issue makes it very tricky to add new states, such as "Disconnecting" for example. Todays domU kernels expect to find the frontend device state in Initializing and the backend device state in Initializing or InitWait. That are the main reasons I went the route to switch back to Initializing state in the frontend, and I see no easy way around that. The backend responds by cleaning up and preparing for the frontend going to "Connected" state again, and sets its own state to "InitWait" to signal that to the frontend. That is a perfectly fine environment for the new kernel. Unfortunaly the old kernel''s frontend reacts on that state change too, which did lead to the introduction of the shutdown_in_progress flag in xenbus_device to avoid that. I hope that are all gotchas I ran into (thats the problem with two weeks vacation between writing code and submitting patches, you forget some of the tricky details ...). cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> http://www.suse.de/~kraxel/julika-dora.jpeg _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/8/06 9:04 am, "Gerd Hoffmann" <kraxel@suse.de> wrote:> * We _must not_ break old domUs.Indeed.> * I''d like to be able to boot old domU kernels via kexec, so the > post-kexec environment should be identical to what the domain > builder creates today.Nice to have. I might be inclined to push support for compat into the kexec layer, though (e.g., fixing up xenstore so that old kernels will be happy), if doing it in core xenbus code seems distasteful.> The backward compatibility issue makes it very tricky to add new states, > such as "Disconnecting" for example. Todays domU kernels expect to find > the frontend device state in Initializing and the backend device state > in Initializing or InitWait. That are the main reasons I went the route > to switch back to Initializing state in the frontend, and I see no easy > way around that.I don''t want to add new XenbusStates. I''m talking about a different, hotplug, status that is rather different: it indicates the status of the device from the p.o.v. Of the admin and control tools (basically, ''online'' or ''offline''). This node has quite a different purpose from the existing state node, which is really a low-level mechanism for sync''ing connection state machines in frontend/backend drivers.> The backend responds by cleaning up and preparing for the frontend going > to "Connected" state again, and sets its own state to "InitWait" to > signal that to the frontend. That is a perfectly fine environment for > the new kernel. Unfortunaly the old kernel''s frontend reacts on that > state change too, which did lead to the introduction of the > shutdown_in_progress flag in xenbus_device to avoid that.Two options: 1. Always enter Closed state, and then run a cleanup phase in the kexec code which iterates over xenstore device directories, switching Closed->Initialising. 2. Enter state Initialising if backend''s ''hotplug_status'' node indicates ''online''. Gate xenbus_probe_node() on kernel_status <= STATE_RUNNING (the shutdown code only ever gets run after kernel_status is bumped to one of the shutdown states, I believe). I think (1) is architecturally cleaner, but I understand it may be a pain to implement in the constrained post-shutdown kexec environment. I don''t think (2) is too bad, and is more along the lines of what you have already. -- Keir> I hope that are all gotchas I ran into (thats the problem with two weeks > vacation between writing code and submitting patches, you forget some of > the tricky details ...)._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/8/06 9:30 am, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote:> Two options: > 1. Always enter Closed state, and then run a cleanup phase in the kexec > code which iterates over xenstore device directories, switching > Closed->Initialising. > 2. Enter state Initialising if backend''s ''hotplug_status'' node indicates > ''online''. Gate xenbus_probe_node() on kernel_status <= STATE_RUNNING (the > shutdown code only ever gets run after kernel_status is bumped to one of the > shutdown states, I believe). > > I think (1) is architecturally cleaner, but I understand it may be a pain to > implement in the constrained post-shutdown kexec environment. I don''t think > (2) is too bad, and is more along the lines of what you have already.I had a chat with Ewan about this. We agree option (2) is sane and likely much less work than option (1). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,> I don''t want to add new XenbusStates. I''m talking about a different, > hotplug, status that is rather different: it indicates the status of the > device from the p.o.v. Of the admin and control tools (basically, ''online'' > or ''offline'').I understand that, this is a related but different issue though.> Two options: > 1. Always enter Closed state, and then run a cleanup phase in the kexec > code which iterates over xenstore device directories, switching > Closed->Initialising. > 2. Enter state Initialising if backend''s ''hotplug_status'' node indicates > ''online''. Gate xenbus_probe_node() on kernel_status <= STATE_RUNNING (the > shutdown code only ever gets run after kernel_status is bumped to one of the > shutdown states, I believe). > > I think (1) is architecturally cleaner, but I understand it may be a pain to > implement in the constrained post-shutdown kexec environment. I don''t think > (2) is too bad, and is more along the lines of what you have already.I''ll try (1). I don''t expect it being that hard, I expect doing that in the old kernel _after_ unregistering the watches should work just fine. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> http://www.suse.de/~kraxel/julika-dora.jpeg _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi,>> 1. Always enter Closed state, and then run a cleanup phase in the kexec >> code which iterates over xenstore device directories, switching >> Closed->Initialising.> I''ll try (1). I don''t expect it being that hard, I expect doing that in > the old kernel _after_ unregistering the watches should work just fine.Here we go, changes: * new helper function xenbus_strstate() for more readable debug output. * some of the DPRINK()''s got some more info to print added (device node, state on state changes). * Both sides will happily enter "Closed" state without deleting the device. * There is a new helper function xenbus_reinit_frontend_devices() for kexec: It unregisteres the watch (to prevent the device from being reinitialized in the old kernel) and moves devices from Closed back to Initialising state. cheers, Gerd -- Gerd Hoffmann <kraxel@suse.de> http://www.suse.de/~kraxel/julika-dora.jpeg _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel