Re: [Qemu-devel] [PATCH v5 26/45] Postcopy page-map-incoming (PMI) structure

[David Gibson]

On Wed, Mar 18, 2015 at 05:58:40PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Fri, Mar 13, 2015 at 01:47:53PM +0000, Dr. David Alan Gilbert wrote:
> > > * David Gibson (address@hidden) wrote:
> > > > On Wed, Feb 25, 2015 at 04:51:49PM +0000, Dr. David Alan Gilbert (git) 
> > > > wrote:
> > > > > From: "Dr. David Alan Gilbert" <address@hidden>
> > [snip]
> > > > > +        mis->postcopy_pmi.state0[BIT_WORD(bitmap_index)] |= 
> > > > > shifted_mask;
> > > > > +    } else {
> > > > > +        mis->postcopy_pmi.state0[BIT_WORD(bitmap_index)] &= 
> > > > > ~shifted_mask;
> > > > > +    }
> > > > > +    if (state & 2) {
> > > > > +        mis->postcopy_pmi.state1[BIT_WORD(bitmap_index)] |= 
> > > > > shifted_mask;
> > > > > +    } else {
> > > > > +        mis->postcopy_pmi.state1[BIT_WORD(bitmap_index)] &= 
> > > > > ~shifted_mask;
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Retrieve the state of the given page
> > > > > + * Note: This version for use by callers already holding the lock
> > > > > + */
> > > > > +static PostcopyPMIState postcopy_pmi_get_state_nolock(
> > > > > +                            MigrationIncomingState *mis,
> > > > > +                            size_t bitmap_index)
> > > > > +{
> > > > > +    bool b0, b1;
> > > > > +
> > > > > +    b0 = test_hpbits(mis, bitmap_index, mis->postcopy_pmi.state0);
> > > > > +    b1 = test_hpbits(mis, bitmap_index, mis->postcopy_pmi.state1);
> > > > > +
> > > > > +    return (b0 ? 1 : 0) + (b1 ? 2 : 0);
> > > > 
> > > > Ugh.. this is a hidden dependency on the PostcopyPMIState enum
> > > > elements never changing value.  Safer to code it as:
> > > >       if (!b0 && !b1) {
> > > >           return POSTCOPY_PMI_MISSING;
> > > >       } else if (...)
> > > >            ...
> > > > 
> > > > and let gcc sort it out.
> > > 
> > > Again, I was trying to make this just the interface; so it doesn't
> > > know or care about the enum mapping; we can change the enum mapping to
> > > the bits without changing this function (or the callers) at all.
> > 
> > So.. I'm not entirely clear what you mean by that.  I think what
> > you're saying is that this function basically returns an arbitrary bit
> > pattern derived from the state maps, and the enum provides the mapping
> > from those bit patterns to meaningful states?
> > 
> > That's.. subtle :/.
> 
> I'm saying that I'd like everywhere to work in terms of the enum; but
> since I can't store the array of enums I need to convert somewhere;
> if I can keep the conversion to only being a couple of functions that know
> about the bit layout and everything else uses those functions, then it
> feels safe/clean.

Hm, yeah, I guess.  It's all a bit confusing, but I see the internal
sense in your scheme.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpZamUVQdtW_.pgp
Description: PGP signature