On Wed, 2020-03-18 at 11:03 +0100, Jan Beulich wrote:
> On 17.03.2020 23:15, David Woodhouse wrote:
> > On Thu, 2020-02-20 at 12:59 +0100, Jan Beulich wrote:
> > > On 07.02.2020 19:04, David Woodhouse wrote:
> > 
> >           ASSERT((pg[i].count_info & ~PGC_extra) == PGC_state_inuse ||
> >                  (pg[i].count_info & ~PGC_extra) == PGC_state_uninitialised);
> > > >            page_set_owner(&pg[i], d);
> > > >            smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
> > > > -        pg[i].count_info = PGC_allocated | 1;
> > > > +        pg[i].count_info |= PGC_allocated | 1;
> > > 
> > > This is too relaxed for my taste: I understand you want to
> > > retain page state, but I suppose other bits would want clearing
> > > nevertheless.
> > 
> > You seem to have dropped the ASSERT immediately before the code snippet
> > you cited, in which arbitrary other contents of count_info are not
> > permitted. I put it back, in its current form after I rebase on top of
> > Paul's commit c793d13944b45d assing PGC_extra.
> 
> But that' only an ASSERT(), i.e. no protection at all in release builds.

An ASSERT does protect release builds. If the rule is that you must
never call assign_pages() with pages that have the other bits in
count_info set, then the ASSERT helps to catch the cases where people
introduce a bug and start doing precisely that, and the bug never
*makes* it to release builds.

What we're debating here is the behaviour of assign_pages() when
someone introduces such a bug and calls it with inappropriate pages.

Currently, the behaviour is that the other flags are silently cleared.
I've seen no analysis that such clearing is correct or desirable. In
fact, for the PGC_state bits I determined that it now is NOT correct,
which is why I changed it.

While I was at it, I let it preserve the other bits — which, again,
should never be set, and which would trigger the ASSERT in debug builds
if it were to happen.

But I'm not tied to that behaviour. It's still a "can never happen"
case as far as I'm concerned. So let's make it look like this:


    for ( i = 0; i < (1 << order); i++ )
    {
        ASSERT(page_get_owner(&pg[i]) == NULL);
        /*
         * Note: Not using page_state_is() here. The ASSERT requires that
         * all other bits in count_info are zero, in addition to PGC_state
         * being appropriate.
         */
        ASSERT((pg[i].count_info & ~PGC_extra) == PGC_state_inuse ||
               (pg[i].count_info & ~PGC_extra) == PGC_state_uninitialised);
        page_set_owner(&pg[i], d);
        smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
        pg[i].count_info = (pg[i].count_info & PGC_state) | PGC_allocated | 1;
        page_list_add_tail(&pg[i], &d->page_list);
    }

OK?