| Message ID | 1504026557-11365-2-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 29, 2017 at 01:09:13PM -0400, Boris Ostrovsky wrote: [...] > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 9fa62d2..e1f7cd2 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1839,7 +1839,7 @@ static int __init find_non_smt(unsigned int node, cpumask_t *dest) > * Scrub all unallocated pages in all heap zones. This function uses all > * online cpu's to scrub the memory in parallel. > */ > -void __init scrub_heap_pages(void) > +static void __init scrub_heap_pages(void) > { Since you now guard against opt_bootscrub in heap_init_late, you should remove the check of opt_bootscrub in this function. With this fixed: Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>> On 29.08.17 at 19:09, <boris.ostrovsky@oracle.com> wrote: > @@ -1970,12 +1970,15 @@ void __init scrub_heap_pages(void) > #ifdef CONFIG_SCRUB_DEBUG > boot_scrub_done = true; > #endif > +} > > - /* Now that the heap is initialized, run checks and set bounds > - * for the low mem virq algorithm. */ > +void __init heap_init_late(void) > +{ > setup_low_mem_virq(); > -} > > + if ( opt_bootscrub ) > + scrub_heap_pages(); > +} Any reason you fully remove that comment? I think the "run checks" part is stale (if it was ever valid in the first place), but the rest could more or less stay. Jan
On 08/30/2017 04:50 AM, Jan Beulich wrote: >>>> On 29.08.17 at 19:09, <boris.ostrovsky@oracle.com> wrote: >> @@ -1970,12 +1970,15 @@ void __init scrub_heap_pages(void) >> #ifdef CONFIG_SCRUB_DEBUG >> boot_scrub_done = true; >> #endif >> +} >> >> - /* Now that the heap is initialized, run checks and set bounds >> - * for the low mem virq algorithm. */ >> +void __init heap_init_late(void) >> +{ >> setup_low_mem_virq(); >> -} >> >> + if ( opt_bootscrub ) >> + scrub_heap_pages(); >> +} > Any reason you fully remove that comment? I think the "run checks" > part is stale (if it was ever valid in the first place), but the rest > could more or less stay. I thought it was pretty clear from the routine's name what it is about to do so I dropped it. I can put it back if you feel it is still needed. -boris
>>> On 30.08.17 at 15:02, <boris.ostrovsky@oracle.com> wrote: > On 08/30/2017 04:50 AM, Jan Beulich wrote: >>>>> On 29.08.17 at 19:09, <boris.ostrovsky@oracle.com> wrote: >>> @@ -1970,12 +1970,15 @@ void __init scrub_heap_pages(void) >>> #ifdef CONFIG_SCRUB_DEBUG >>> boot_scrub_done = true; >>> #endif >>> +} >>> >>> - /* Now that the heap is initialized, run checks and set bounds >>> - * for the low mem virq algorithm. */ >>> +void __init heap_init_late(void) >>> +{ >>> setup_low_mem_virq(); >>> -} >>> >>> + if ( opt_bootscrub ) >>> + scrub_heap_pages(); >>> +} >> Any reason you fully remove that comment? I think the "run checks" >> part is stale (if it was ever valid in the first place), but the rest >> could more or less stay. > > I thought it was pretty clear from the routine's name what it is about > to do so I dropped it. I can put it back if you feel it is still needed. I'd prefer if it was (roughly) kept as comment on just the setup_low_mem_virq() invocation (i.e. not the new function as a whole). Jan
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 3b34855..92f173b 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -861,8 +861,7 @@ void __init start_xen(unsigned long boot_phys_offset, if ( construct_dom0(dom0) != 0) panic("Could not set up DOM0 guest OS"); - /* Scrub RAM that is still free and so may go to an unprivileged domain. */ - scrub_heap_pages(); + heap_init_late(); init_constructors(); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index ec96287..bc466e8 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1662,8 +1662,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) cr4_pv32_mask |= X86_CR4_SMAP; } - /* Scrub RAM that is still free and so may go to an unprivileged domain. */ - scrub_heap_pages(); + heap_init_late(); init_trace_bufs(); diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 9fa62d2..e1f7cd2 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1839,7 +1839,7 @@ static int __init find_non_smt(unsigned int node, cpumask_t *dest) * Scrub all unallocated pages in all heap zones. This function uses all * online cpu's to scrub the memory in parallel. */ -void __init scrub_heap_pages(void) +static void __init scrub_heap_pages(void) { cpumask_t node_cpus, all_worker_cpus; unsigned int i, j; @@ -1970,12 +1970,15 @@ void __init scrub_heap_pages(void) #ifdef CONFIG_SCRUB_DEBUG boot_scrub_done = true; #endif +} - /* Now that the heap is initialized, run checks and set bounds - * for the low mem virq algorithm. */ +void __init heap_init_late(void) +{ setup_low_mem_virq(); -} + if ( opt_bootscrub ) + scrub_heap_pages(); +} /************************* diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index ddc3fb3..c2f5a08 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -199,7 +199,7 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status); int query_page_offline(unsigned long mfn, uint32_t *status); unsigned long total_free_pages(void); -void scrub_heap_pages(void); +void heap_init_late(void); int assign_pages( struct domain *d,
scrub_heap_pages() does early return if boot-time scrubbing is disabled, neglecting to initialize lowmem VIRQ. Because setup_low_mem_virq() doesn't logically belong in scrub_heap_pages() we put them both into the newly added heap_init_late(). Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- CC: Julien Grall <julien.grall@arm.com> --- Changes in v2: * Added heap_init_late() which calls setup_low_mem_vir() and scrub_heap_pages() xen/arch/arm/setup.c | 3 +-- xen/arch/x86/setup.c | 3 +-- xen/common/page_alloc.c | 11 +++++++---- xen/include/xen/mm.h | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-)