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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, May 5, 2022 8:07 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>;
> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> 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.
>

Ok. Thanks for explanation.
I misunderstand what you suggested here, I thought you were suggesting a way of 
stub function
which could bring some optimization.
The reason I introduced free_staticmem_pages and acquire_reserved_page here is 
that
we now used them in common code, and if they are not defined(using stub) on 
!CONFIG_STATIC_MEMORY,
we will have " hidden symbol `xxx' isn't defined " compilation error.
 
> Jan