Re: [PATCH 4/9] xen/arm: static memory initialization

[Jan Beulich <jbeulich@suse.com>]

On 05.07.2021 09:14, Penny Zheng wrote:
>> From: Penny Zheng
>> Sent: Monday, July 5, 2021 1:22 PM
>>
>>> From: Julien Grall <julien@xen.org>
>>> Sent: Thursday, July 1, 2021 1:46 AM
>>>
>>> On 10/06/2021 10:35, Jan Beulich wrote:
>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
>>>>>       spin_unlock(&heap_lock);
>>>>>   }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/* Equivalent of free_heap_pages to free nr_mfns pages of static
>>>>> +memory. */ void __init free_staticmem_pages(struct page_info *pg,
>>> unsigned long nr_mfns,
>>>>> +                                 bool need_scrub) {
>>>>> +    mfn_t mfn = page_to_mfn(pg);
>>>>> +    unsigned long i;
>>>>> +
>>>>> +    for ( i = 0; i < nr_mfns; i++ )
>>>>> +    {
>>>>> +        switch ( pg[i].count_info & PGC_state )
>>>>> +        {
>>>>> +        case PGC_state_inuse:
>>>>> +            BUG_ON(pg[i].count_info & PGC_broken);
>>>>> +            /* Mark it free and reserved. */
>>>>> +            pg[i].count_info = PGC_state_free | PGC_reserved;
>>>>> +            break;
>>>>> +
>>>>> +        default:
>>>>> +            printk(XENLOG_ERR
>>>>> +                   "Page state shall be only in PGC_state_inuse. "
>>>>
>>>> Why? A page (static or not) can become broken while in use. IOW I
>>>> don't think you can avoid handling PGC_state_offlining here. At
>>>> which point this code will match free_heap_pages()'es, and hence
>>>> likely will want folding as well.
>>>>
>>
>> Yeah, I was following the logic in free_heap_pages.
>> Hmmm, I could not think of any scenario that will lead to PGC_state_offlining,
>> that's why I was not including it at the first place.
>> For broken issue, tbh, I just copy the bug_on from free_heap_pages, after
>> quite a time thinking, I also could not find any scenario when a page(static or
>> not) can become broken while in use. ;/

I'm, afraid I don't understand. Using page_to_mfn(), expensive or not,
in ASSERT() is quite fine. The (expensive) expression won't be evaluated
in release builds. This is specifically different from BUG_ON().

Jan