Re: [Xen-devel] [PATCH v3 COLOPre 06/26] libxl/save: Refactor libxl__domain_suspend_state

[Yang Hongyang <yanghy@xxxxxxxxxxxxxx>]

On 06/30/2015 05:50 PM, Ian Campbell wrote:
> On Tue, 2015-06-30 at 17:43 +0800, Yang Hongyang wrote:
> > On 06/30/2015 12:01 AM, Ian Campbell wrote:
> > > On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
> > > [...]
> > > >        switch (type) {
> > > >        case LIBXL_DOMAIN_TYPE_HVM: {
> > > >            dss->hvm = 1;
> > > > +        dsps->hvm = 1;
> > > >            break;
> > > >        }
> > > >        case LIBXL_DOMAIN_TYPE_PV:
> > > >            dss->hvm = 0;
> > > > +        dsps->hvm = 0;
> > > >            break;
> > > [...]
> > > > @@ -2913,9 +2914,27 @@ typedef struct libxl__logdirty_switch {
> > > >    } libxl__logdirty_switch;
> > > >
> > > >    struct libxl__domain_suspend_state {
> > > > +    /* set by caller of domain_suspend_callback_common */
> > > > +    libxl__ao *ao;
> > > > +
> > > > +    uint32_t domid;
> > > > +    int hvm;
> > > > +    /* private */
> > > > +    libxl__ev_evtchn guest_evtchn;
> > > > +    int guest_evtchn_lockfd;
> > > > +    int guest_responded;
> > > > +    libxl__xswait_state pvcontrol;
> > > > +    libxl__ev_xswatch guest_watch;
> > > > +    libxl__ev_time guest_timeout;
> > > > +    const char *dm_savefile;
> > > > +    void (*callback_common_done)(libxl__egc*,
> > > > +                                 struct libxl__domain_suspend_state*, int ok);
> > > > +};
> > > > +
> > > > +struct libxl__domain_save_state {
> > > >        /* set by caller of libxl__domain_suspend */
> > > >        libxl__ao *ao;
> > > > -    libxl__domain_suspend_cb *callback;
> > > > +    libxl__domain_save_cb *callback;
> > > >
> > > >        uint32_t domid;
> > > >        int fd;
> > > > @@ -2924,22 +2943,14 @@ struct libxl__domain_suspend_state {
> > > >        int debug;
> > > >        const libxl_domain_remus_info *remus;
> > > >        /* private */
> > > > -    libxl__ev_evtchn guest_evtchn;
> > > > -    int guest_evtchn_lockfd;
> > > > +    libxl__domain_suspend_state dsps;
> > > >        int hvm;
> > > I wonder if, given that any domain suspend must necessarily be contained
> > > within a domain save it would be preferable to have "suspend" code
> > > upcast the suspend_state to the containing save_state in order to look
> > > at ->hvm, rather than duplicating it. Likewise domid.
> > The reason we include hvm in suspend state is that we need a more common
> > suspend function we will use on restore side (with COLO). It should not touch
> > the upper struct, because it will be used on both save/restore side.
> OK makes sense, thanks. You may as well mention this in the commit
> message so I don't forget next time ;-)
Ok, will do :)

> > > For ao I can imagine that the suspend and save might actually have
> > > separate ao lifetimes, in which case these do of course need to remain
> > > different.
> > >
> > > Would that make any sense at all?
> > >
> > > FYI it was the dual initialisation of hvm quoted above which lead me to
> > > think along these lines.
> > >
> > > Alternatively perhaps suspend_state should a separate init function to
> > > logically separate it from the save_state initialiser. Perhaps taking
> > > the latter as an argument? Or possibly just the relevant fields.
> > Yes, the latter should be better. I will separate the init of the
> > suspend state.
> Thanks.
>
>
> .
--
Thanks,
Yang.

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