Re: [Xen-devel] [PATCH v16 2/7] remus: introduce remus device

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

Hi Ian,

  There're two comments left that I wanted to comment further on.
Apart from this two comments, I think I have addressed all your
comments on v16, but before I send a v18 out, Shriram and I will
have more detailed review on it, in case I have missed some of
your comments.

My latest working tree that we will review on is hosted at github:
https://github.com/macrosheep/xen/tree/remus-v17.6
Refer to the latest 10 commits.
What I think is the big changes compared to v16:
 - Merge match() and setup() api.
 - Reuse libxl__multidev and libxl__ao_device

NOTE:
  branches which has `draft` in the name may not compile, and you can
simply ignore it.
  branches which has version number like remus-vXX is compilable and
well tested, you can keep on track on those versions, especially the
latest version makes sense.

On 07/16/2014 07:46 PM, Ian Jackson wrote:
> > > > +typedef enum libxl__remus_device_kind {
> > > > +    LIBXL__REMUS_DEVICE_NIC,
> > > > +    LIBXL__REMUS_DEVICE_DISK,
> > > > +} libxl__remus_device_kind;
> > > AFAICT this enum is used only for filtering which subkind ops vtables
> > > we attempt setup() on.  Maybe it would be better to remove this enum
> > > and replace its uses in libxl__remus_devices_setup with references to
> > > (lists of) vtables ?
> ...
> > > Eg
> > >       for (i = 0; i < rds->num_nics; i++) {
> > >               libxl__remus_device_init(egc, rds,
> > >                                        libxl__remus_device_subkinds_nic,
> > >                                        &rds->nics[i]);
> ...
> > Thanks for the detailed comments. this enum also used for match() and
> > other uses, so I will keep it.
> My proposal above would do away with the need for match to look at the
> kind at all.  It would only iterate over the subkinds which are
> relevant to that kind.
>
> What are the other purposes you mention ?  I didn't notice them.
I intend to keep this *kind* because I will use them for filter out
what kind of device the remus abstract layer need to handle.

take the code v17.6(refer to above link) as an example:

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

there's a flag in remus devices state(which I will call rds below):
struct libxl__remus_devices_state {
...
    int device_kind_flags;
...
}

and this flag will be set by upper layer(remus save/restore) depend
on the comand switch:
int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
                             uint32_t domid, int send_fd, int recv_fd,
                             const libxl_asyncop_how *ao_how)
{
...
    if (info->netbuf) {
        if (!libxl__netbuffer_enabled(gc)) {
            LOG(ERROR, "Remus: No support for network buffering");
            goto out;
        }
        rds->device_kind_flags |= LIBXL__REMUS_DEVICE_NIC;
    }

    if (info->diskbuf)
        rds->device_kind_flags |= LIBXL__REMUS_DEVICE_DISK;
...
}

so that I can determine what kind of device I need to handle in
remus abstract layer without had to know command switch related things.
I think the fact that the abstract layer need to know what the command
switch is causes layer violation...

void libxl__remus_devices_setup(libxl__egc *egc, libxl__remus_devices_state 
*rds)
{
...
    if (rds->device_kind_flags & LIBXL__REMUS_DEVICE_NIC)
        rds->nics = libxl_device_nic_list(CTX, rds->domid, &rds->num_nics);

    if (rds->device_kind_flags & LIBXL__REMUS_DEVICE_DISK)
        rds->disks = libxl_device_disk_list(CTX, rds->domid, &rds->num_disks);
...
}

> > > > +/*----- helper functions -----*/
> > > > +
> > > > +static int init_device_subkind(libxl__remus_device_state *rds)
> > > > +{
> > > > +    int rc;
> > > > +    const libxl__remus_device_subkind_ops **ops;
> > > > +
> > > > +    for (ops = rds->ops; *ops; ops++) {
> > > > +        rc = (*ops)->init(rds);
> > > Why not call init immediately before match() ?  That would do away
> > > with this loop entirely.
> > >
> > > Furthermore, this seems to imply that init is idempotent.  This isn't
> > > spelled out in your API comments.
> > >
> > > If init is idempotent, and is going to be called before match() every
> > > time, why not fold it into match ?
> > a ops will be called many times in match(). but init()/destroy() only
> > need to be called once. so I think it's better to separate it.
> init is in fact called more than once.  Is it supposed to be
> idempotent ?  If it is supposed idempotent then it can check at the
> start if anything needs to be done.  That would make the lifetime of
> the per-subkkind state a matter entirely for the subkind, which would
> simplify the API.  And, I think, the code.
>
> > and also if a subkind init failed, we can exit immediately, and not
> > need to wait for match complete.
> I don't understand why this is important.  Performance isn't important
> here, is it ?  And since we have to have the code for dealing with
> asynchronous matching anyway, we don't gain any code simplicity from
> separating init out.
a subkind init()/cleanup() only need to be called once per Remus instance.
there're cases that this subkind is used for multiple devices, it we don't
seperate it out, it will be called many times. I agree that I can make
it capable to be called multiple times, but I think that's not what the
design is. it is not supposed to be idempotent I think. and I think
separate it makes the logic more comprehensive.

Thanks,
Yang

--
Thanks,
Yang.

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