Re: [Xen-devel] [PATCH v18 04/11] libxl/remus: introduce an abstract Remus device layer

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[PATCH v18 04/11] libxl/remus: introduce an abstract 
Remus device layer"):
> Introduce an abstract device layer that allows the Remus
> logic in libxl to control a guest's devices in a device-agnostic
> manner. The device layer also exposes a set of internal interfaces
> that a device type must implement, if it wishes to support Remus.

Thanks.  I think this is converging.  I have mostly nits as comments
now.  I have only two nontrivial comments: one about your use of
multidev which I think needs to be improved, and the other is about
the libxl__remus_device_kind enum (which you are already aware of).



> +static void remus_devices_preresume_cb(libxl__egc *egc,
> +                                       libxl__remus_devices_state *rds,
> +                                       int rc)
> +{
> +    int ok = 0;
> +    libxl__domain_suspend_state *dss = CONTAINER_OF(rds, *dss, rds);
> +    STATE_AO_GC(dss->ao);
> +
> +    if (rc)
>          goto out;
>  
> -    /* REMUS TODO: Deal with disk. Start a new network output buffer */
> -    ok = 1;
> +    /* Resumes the domain and the device model */
> +    if (!libxl__domain_resume(gc, dss->domid, /* Fast Suspend */1))
> +        ok = 1;

Again, this should use the standard `goto out' error handling style.
In this case that means:

     rc = libxl__domain_resume(gc, dss->domid, /* Fast Suspend */1);
     if (rc) goto out;

     ok = 1;
   out:


> +static void remus_devices_commit_cb(libxl__egc *egc,
> +                                    libxl__remus_devices_state *rds,
> +                                    int rc)
> +{
...
> +    /* Set checkpoint interval timeout */
> +    rc = libxl__ev_time_register_rel(gc, &dss->checkpoint_timeout,
> +                                     remus_next_checkpoint,
> +                                     dss->interval);
> +
> +    if (rc) {
> +        LOG(ERROR, "unable to register timeout for next epoch."
> +            " Terminating Remus..");
> +        goto out;
> +    }

There is no need to log failures of libxl__ev_time_register_rel et
al.  See the comment in libxl_internal.h near line 691.  It is
sufficient to do

      if (rc) goto out;

> +typedef enum libxl__remus_device_kind {
> +    LIBXL__REMUS_DEVICE_NIC  = (1 << 0),
> +    LIBXL__REMUS_DEVICE_DISK = (1 << 1),
> +} libxl__remus_device_kind;

We still need to talk about this, and the comments I had about the
vtables.

> +typedef struct libxl__remus_device libxl__remus_device;
> +typedef struct libxl__remus_devices_state libxl__remus_devices_state;
> +typedef struct libxl__remus_device_subkind_ops 
> libxl__remus_device_subkind_ops;
> +
> +/*
> + * Interfaces to be implemented by every device type that wishes to
> + * support Remus. Functions must be implemented unless otherwise
> + * stated. Many of these functions are asynchronous. They call
> + * dev->aodev.callback when done.  The actual implementations may be
> + * synchronous and call dev->aodev.callback directly (as the last
> + * thing they do).
> + */
> +struct libxl__remus_device_subkind_ops {
> +    /* the device kind this ops belongs to... */
> +    libxl__remus_device_kind kind;
> +
> +    /*
> +     * init() and cleanup() relate to the subkind-specific state in
> +     * the libxl ctx, not to any specific device.
> +     * Synchronous. cleanup() cannot fail.
> +     */
> +    int (*init)(libxl__remus_devices_state *rds);
> +    void (*cleanup)(libxl__remus_devices_state *rds);

But actually they take a libxl__remus_devices_state.

Either the state is global for all simultaneous remus invocations in
with this libxl_ctx, in which case init and cleanup should not take
any libxl__remus_devices_state.

Or the state is per remus invocation, in which case the comment is
wrong.

You also need to document the error behaviour.  From the call site I
think something like:

     Before the first call to init, the subkind-specific state will be
     all-bits-zero.  cleanup will be called whether or not init
     succeeded.

This is a similar situation to the one where I asked you to document
the same thing about `teardown'.


And if this is global state in the libxl_ctx, you have to also say:

     init must be idempotent; it will be called multiple times,
     possibly even if after it has been called and failed.

And if that is the semantics I think something like `ensure_inited' is
probably correct for its name.


> +    int num_devices;
> +    /*
> +     * this array is allocated before setup the remus devices by the
> +     * remus abstract layer.
> +     * the size of this array is 'num_devices', which is the total number
> +     * of libxl nic devices and disk devices(num_nics + num_disks).
> +     */
> +    libxl__remus_device **dev;

(As I said before) this comment leaves some questions unananswered:

What proportion of the devs array is initialised at any one time ?
May the devs array contain null pointers and what do they mean ?  etc.

(And, sorry for not noticing this last time, but I think this variable
needs to be called `devs' rather than `dev'.)

> +/*
> + * Information about a single device being handled by remus.
> + * Allocated by the remus abstract layer.
> + */
> +struct libxl__remus_device {
> +    /*----- shared between abstract and concrete layers -----*/
> +    /*
> +     * if this is true, that means the subkind ops matched the
> +     * device and we have actually set up the device no matter
> +     * setup succeed or not.
> +     */
> +    int set_up;

I don't understand this.  The protocol documented in
libxl__remus_device_subkind_ops seems to be how the subkind
communicates to the abstract layer whether the device was successfully
set up.  Is this variable in fact solely for the abstract layer ?

Also, "we have actually set up the device" and "setup succeeded" seem
to be the same thing.

(Also, can it be a boolean?)

> +    /* find the error that was not ERROR_REMUS_DEVOPS_DOES_NOT_MATCH */
> +    for (i = 0; i < rds->num_devices; i++) {
> +        dev = rds->dev[i];
> +
> +        if (!dev->aodev.rc || dev->aodev.rc == 
> ERROR_REMUS_DEVOPS_DOES_NOT_MATCH)

This is quite tortuous.  I think you probably want to do it
differently by having two layers of callback function:

You should probably make the multidev->callback only when you have
found the right subkind (or failed).

So the subkind should be told to use a different callback which is
handled here in the abstract type code.  Then your abstract code can
iterate separately through each subkind, rather than hunting through
the innards of multidev.

(I think that accessing aodev->rc here is a layering violation.)


Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel