Re: [Xen-devel] [PATCH v6 08/15] x86/efi: create new early memory allocator

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.09.16 at 20:45, <daniel.kiper@xxxxxxxxxx> wrote:
> On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
>> >>> On 20.09.16 at 12:52, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote:
>> >> >>> On 20.09.16 at 11:45, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
>> >> >> >>> On 19.09.16 at 17:04, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> >> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
>> >> >> >> >>> On 12.09.16 at 22:18, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> >> >> > --- a/xen/arch/x86/setup.c
>> >> >> >> > +++ b/xen/arch/x86/setup.c
>> >> >> >> > @@ -520,6 +520,8 @@ static void noinline init_done(void)
>> >> >> >> >
>> >> >> >> >      system_state = SYS_STATE_active;
>> >> >> >> >
>> >> >> >> > +    free_ebmalloc_unused_mem();
>> >> >> >>
>> >> >> >> Now that the allocator properly lives in common code, this appears
>> >> >> >> to lack an ARM side counterpart.
>> >> >> >
>> >> >> > Why? It is called only from xen/arch/x86/setup.c:__start_xen() and 
>> >> >> > all
>> >> >> > ebmalloc stuff is in #ifndef CONFIG_ARM. So, 
>> >> >> > free_ebmalloc_unused_mem()
>> >> >> > will be needed only if we add ARM support here.
>> >> >>
>> >> >> Well, it being inside that conditional is part of the problem - there's
>> >> >> no apparent point for all of it to be.
>> >> >
>> >> > I can agree that this is potentially generic stuff (well, I understand 
>> >> > that
>> >> > it is our final goal but unreachable yet due to various things). 
>> >> > However,
>> >> > right know it is only used on x86. So, I am not sure what is the problem
>> >> > with #ifndef CONFIG_ARM right now...
>> >>
>> >> It is a fact that these should actually not be there, so we ought to
>> >> at least limit them to the smallest possible count and scopes.
>> >>
>> >> >> Arguably the one static function may better be, as other workarounds
>> >> >> to avoid the "unused" compiler warning wouldn't be any better.
>> >> >
>> >> > Do you mean static function with empty body for ARM? It is not needed
>> >> > right now because it is never called on ARM. Am I missing something?
>> >>
>> >> You misunderstood - I said that for this one (unused) static
>> >> function such an #ifdef is probably okay, as the presumably
>> >> smallest possible workaround.
>> >
>> > Do you suggest that I should move out of #ifndef CONFIG_ARM all ebmalloc 
>> > stuff
>> > except free_ebmalloc_unused_mem(). Even if it is not used on ARM right now?
>>
>> That's not the static function, is it? The function you name should
>> actually be called on ARM too (as I did point out elsewhere already),
>> just to not leave a trap open for someone to fall into when (s)he
>> adds a first use of the allocator on ARM.
> 
> Well, I think that sane person doing that would analyze how ebmalloc works
> on x86 and then move (align to ARM needs if required) all its machinery
> (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do
> that. This way he/she would avoid issues mentioned by you. However, if you
> still prefer your way I can do that. Though I am not sure about the rest of
> ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your
> earlier replies I see that yes. Am I correct?

Yes.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel