[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] page_alloc: assert IRQs are enabled in heap alloc/free
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?
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |