Re: [PATCH v3 COLOPre 19/26] libxc/migration: Specification update for DIRTY_BITMAP records

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-07-01 at 11:39 +0100, Andrew Cooper wrote:
> On 01/07/15 11:27, Ian Campbell wrote:
> > On Wed, 2015-07-01 at 11:16 +0100, Andrew Cooper wrote:
> >> On 01/07/15 04:07, Yang Hongyang wrote:
> >>>
> >>> On 06/30/2015 06:24 PM, Ian Campbell wrote:
> >>>> On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
> >>>>> Used by secondary to send it's dirty bitmap to primary under COLO.
> >>>> This is the backchannel, right?
> >>> Right.
> >>>
> >>>> It seems to me that this ought to be described more clearly as a
> >>>> separate stream in the opposite direction, rather than looking like just
> >>>> another record in the forward channel.
> >>> Agreed, I'm not sure if having this back channel record is eligible,
> >>> Andy, thoughts?
> >>>
> >>>> Does the back channel not also need some sort of negotiation phase where
> >>>> we check both ends are compatible (i.e. like the forward channel's
> >>>> header). This might be easier than with the forward channel since you
> >>>> might assert that the versions must match exactly for COLO to be
> >>>> possible, that might not be true of some potential future user of the
> >>>> backchannel though.
> >>> The negotiation record for COLO is introduced in the following patch
> >>> on libxl side. But that might be diffrent form what you said here, we
> >>> don't have a version check currently, if the 2 side doesn't match, for
> >>> example one has colo feature enabled and the other end do not, the
> >>> migration will simply fail.
> >> I do think that each backchannel level needs some kind of initial
> >> negotiation to confirm everything is set up and working, but I think the
> >> backchannel should also match the spec for its level, and all contained
> >> in the single spec document.
> > In the same spec, sure. It's the presenting it as just another record
> > mixed in with all the others which I think is a problem.
> 
> Ah I see.  Yes - this would better be avoided.
> 
> >
> > At the very least every record should be tagged as either forward,
> > backward or bidirectional to indicate who can produce and who should
> > consume it.
> >
> > Even better would br if we can convince ourselves there should be no
> > bidirectional fields, in which case I'd be further inclined to say that
> > the record space should be explicitly separate. i.e. the backchannel
> > should be a separate chapter in the doc and the records.
> 
> I think it would be unwise to rule out the possibility of bidirectional
> records.  In the case that we get to a position of wanting/needing them,
> we absolutely don't want a bidirectional record to have different id in
> the forwards and backwards direction.

Agreed.

Perhaps we should reserve some space for forward/backward/bidir records
in the record id space? Bit 31 is already the optional flag so e.g.
perhaps bit 30 = Backwards and bit 29 = !Forwards.

So a forward mandatory id would be 0x0......., backward would be
0x4....... and bidir would be 0x6......., optional bidir would be
0xe....... etc?

The weird inversion of Forward is in order to retain the existing record
ids.

> 
> ~Andrew
> 
> >
> >> So for both the libxc and libxl backchannels, I would have thought
> >> something along these lines to be sensible:
> >>
> >> Forwards sends a LIBX{C,L}_BACKCHANNEL_INIT, and waits to find a
> >> LIBX{C,L}_BACKCHANNEL_REPLY on the backchannel.  After that, processing
> >> continues as normal, with records arriving on the backchannel when
> >> appropriate.
> > Yes, for init time something like this sounds sensible.
> >
> > Ian.
> >
>