Re: [Xen-devel] [PATCH 2/2] xen/mm: Introduce PG_state_uninitialised

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2020 13:11, David Woodhouse wrote:
> 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?

Yes, thanks.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel