Re: [PATCH v2] page_alloc: assert IRQs are enabled in heap alloc/free

[David Vrabel <dvrabel@xxxxxxxxxx>]

On 21/04/2022 12:38, Jan Beulich wrote:
> On 21.04.2022 12:43, David Vrabel wrote:
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -984,6 +984,8 @@ void __init start_xen(unsigned long boot_phys_offset,
> >   
> >       console_init_postirq();
> >   
> > +    system_state = SYS_STATE_smp_boot
> > +
> >       do_presmp_initcalls();
> >   
> >       for_each_present_cpu ( i )
> I'm afraid it's not this simple: There are two
> "ASSERT(system_state != SYS_STATE_boot)" in Arm-specific code. While
> both could in principle be left as is, I think both want modifying to
> ">= SYS_STATE_active", such that they would also trigger when in this
> newly set state (in case registration of the notifiers was altered).
These asserts are already too-relaxed given that there's an early_boot 
state.
> It also wants at least mentioning that setting this state is okay with
> all uses of system_state in common code (where it's not impossible
> that x86-isms still exist, having gone unnoticed so far), just to
> indicate that all of these were actually inspected (there's just one
> where it looks to be unobvious when simply looking at grep output, the
> one in keyhandler.c). As a result this may want to be a separate,
> prereq patch. At which point it will want considering whether to put
> the setting of the state _in_ do_presmp_initcalls() instead of ahead
> of its invocation.
Not sure I understand this comment. The transition to the smp_boot state 
on arm makes the state machine on x86 and arm look _more_ alike, thus 
common code should be happier.
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -162,6 +162,14 @@
> >   static char __initdata opt_badpage[100] = "";
> >   string_param("badpage", opt_badpage);
> >   
> > +/*
> > + * Heap allocations may need TLB flushes which require IRQs to be
> > + * enabled (except during early boot when only 1 PCPU is online).
> > + */
> > +#define ASSERT_ALLOC_CONTEXT()                                          \
> > +    ASSERT(!in_irq() && (local_irq_is_enabled()                         \
> > +                         || system_state < SYS_STATE_smp_boot))
> Upon further consideration: In principle IRQs would be okay to be off
> whenever we're in UP mode (and hence flush IPIs don't need sending).
> Provided of course spin debug is off as well and no other IRQs-on
> checks get in the way (like that in flush_area_mask()). This might be
> more robust overall than depending on system_state, but I'm not going
> to exclude there may also be arguments against doing so.
Not sure I understand what you're suggesting here. Do you mean something 
like this?
#define ASSERT_ALLOC_CONTEXT()                                         \
    ASSERT(!in_irq() && (local_irq_is_enabled()                        \
                         || nr_online_cpus == 1))

> In any event, looking back at my v1 comment, it would have been nice
> if the spinlock related aspect was at least also mentioned here, even
> if - as you did say in reply - the uses of the new macro aren't fully
> redundant with check_lock().
>
> Also, nit: The || belongs on the earlier line as per our coding style.
CODING_STYLE says: "Long lines should be split at sensible places and 
the trailing portions indented."
If you're going to have rules (that have, IMO[1], worse readability) 
please document them.
David

[1] Compare

a = b
    + dksaldksa_daskldsa_dsakdlsad
    + hds
    + dsadjka_jdaksjdk_daskajd;

and

a = b +
    dksaldksa_daskldsa_dsakdlsad +
    hds +
    dsadjka_jdaksjdk_daskajd;

Which one is more clearly readable as a sum?