Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2022 11:29, Penny Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Thursday, May 5, 2022 4:51 PM
>>
>> On 05.05.2022 10:44, Penny Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Thursday, May 5, 2022 3:47 PM
>>>>
>>>> On 05.05.2022 08:24, Penny Zheng wrote:
>>>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> Sent: Wednesday, May 4, 2022 9:45 PM
>>>>>>
>>>>>> On 27.04.2022 11:27, Penny Zheng wrote:
>>>>>>>  #else
>>>>>>>  void free_staticmem_pages(struct page_info *pg, unsigned long
>>>> nr_mfns,
>>>>>>>                            bool need_scrub)  {
>>>>>>>      ASSERT_UNREACHABLE();
>>>>>>>  }
>>>>>>> +
>>>>>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>>>>>>> +                                   unsigned int nr_mfns, unsigned
>>>>>>> +int
>>>>>>> +memflags) {
>>>>>>> +    ASSERT_UNREACHABLE();
>>>>>>> +}
>>>>>>
>>>>>> I can't spot a caller of this one outside of suitable #ifdef. Also
>>>>>> the __init here looks wrong and you look to have missed dropping it
>>>>>> from
>>>> the real function.
>>>>>>
>>>>>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
>>>>>>> +memflags) {
>>>>>>> +    ASSERT_UNREACHABLE();
>>>>>>> +}
>>>>>>>  #endif
>>>>>>
>>>>>> For this one I'd again expect CSE to leave no callers, just like in
>>>>>> the earlier patch. Or am I overlooking anything?
>>>>>>
>>>>>
>>>>> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only
>>>>> variables, like
>>>>> d->resv_page_list, so I'd expect to let acquire_reserved_page
>>>>> d->guarded by CONFIG_STATIC_MEMORY
>>>>> too and provide the stub function here to avoid compilation error
>>>> when !CONFIG_STATIC_MEMORY.
>>>>
>>>> A compilation error should only result if there's no declaration of
>>>> the function. I didn't suggest to remove that. A missing definition
>>>> would only be noticed when linking, but CSE should result in no
>>>> reference to the function in the first place. Just like was suggested
>>>> for the earlier patch. And as opposed to the call site optimization
>>>> the compiler can do, with -ffunction-sections there's no way for the linker
>> to eliminate the dead stub function.
>>>>
>>>
>>> Sure, plz correct me if I understand wrongly:
>>> Maybe here I should use #define xxx to do the declaration, and it will
>>> also avoid bringing dead stub function. Something like:
>>> #define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg),
>>> (void)(nr_mfns), (void)(need_scrub)) And #define
>>> acquire_reserved_page(d, memflags) (INVALID_MFN)
>>
>> No, I don't see why you would need #define-s. You want to have normal
>> declarations, but no definition unless STATIC_MEMORY. If that doesn't work,
>> please point out why (i.e. what I am overlooking).
>>
> 
> I was trying to avoid dead stub function, and I think #define-s is the wrong 
> path...
> So, I guess If I remove the ASSERT_UNREACHABLE() part and only leave the empty
> function body there, the CSE could do the optimization and result in no 
> reference.

No, DCE (I'm sorry for the earlier wrong uses of CSE) cannot eliminate a
function, it can only eliminate call sites. Hence it doesn't matter whether
a function is empty. And no, if a stub function needs retaining, the
ASSERT_UNREACHABLE() should also remain there if the function indeed is
supposed to never be called.

Jan