Re: [PATCH RFC] MIPS: Build uasm-generated code only once to avoid CPU Hotplug problem

[David Daney <ddaney.cavm@gmail.com>]

On 03/04/2013 04:56 AM, Huacai Chen wrote:
> Currently, clear_page()/copy_page() are generated by Micro-assembler
> dynamically. But they are unavailable until uasm_resolve_relocs() has
> finished because jump labels are illegal before that. Since these
> functions are shared by every CPU, we only call build_clear_page()/
> build_copy_page() on CPU#0 at boot time. Without this patch, programs
> will get random memory corruption (segmentation fault, bus error, etc.)
> while CPU Hotplug (e.g. one CPU is using clear_page() while another is
> generating it in cpu_cache_init()).
>
> For similar reasons we modify build_tlb_refill_handler()'s invocation.
>

I like the general idea behind the patch.  But would prefer to move the 
test to the functions that are generating the code.  That way if more 
call sites are added, we don't have to copy the boiler-plate testing code.

Also can we change the test to something like:

    static atomic_t execute_once;
    .
    .
    .
    if (!atomic_xchg(&execute_once, 1)) {
       /* Generate the code */
    }
    .
    .
    .

That way if we take CPU-0 on and off line, the code will not be regenerated.

Thanks,
David Daney


> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> Signed-off-by: Hongbing Hu <huhb@lemote.com>
> ---
>   arch/mips/mm/c-octeon.c        |    6 ++++--
>   arch/mips/mm/c-r3k.c           |    6 ++++--
>   arch/mips/mm/c-r4k.c           |    6 ++++--
>   arch/mips/mm/c-tx39.c          |    6 ++++--
>   arch/mips/mm/tlb-r3k.c         |    3 ++-
>   arch/mips/mm/tlb-r4k.c         |    3 ++-
>   arch/mips/mm/tlb-r8k.c         |    3 ++-
>   arch/mips/sgi-ip27/ip27-init.c |    3 ++-
>   8 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/arch/mips/mm/c-octeon.c b/arch/mips/mm/c-octeon.c
> index 6ec04da..181a1bc 100644
> --- a/arch/mips/mm/c-octeon.c
> +++ b/arch/mips/mm/c-octeon.c
> @@ -280,8 +280,10 @@ void __cpuinit octeon_cache_init(void)
>
>   	__flush_kernel_vmap_range	= octeon_flush_kernel_vmap_range;
>
> -	build_clear_page();
> -	build_copy_page();
> +	if (smp_processor_id() == 0) {
> +		build_clear_page();
> +		build_copy_page();
> +	}
>
>   	board_cache_error_setup = octeon_cache_error_setup;
>   }
> diff --git a/arch/mips/mm/c-r3k.c b/arch/mips/mm/c-r3k.c
> index 031c4c2..b7b0cfd 100644
> --- a/arch/mips/mm/c-r3k.c
> +++ b/arch/mips/mm/c-r3k.c
> @@ -342,6 +342,8 @@ void __cpuinit r3k_cache_init(void)
>   	printk("Primary data cache %ldkB, linesize %ld bytes.\n",
>   		dcache_size >> 10, dcache_lsize);
>
> -	build_clear_page();
> -	build_copy_page();
> +	if (smp_processor_id() == 0) {
> +		build_clear_page();
> +		build_copy_page();
> +	}
>   }
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index 5f9171d..3f671d6 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -1548,8 +1548,10 @@ void __cpuinit r4k_cache_init(void)
>   	}
>   #endif
>
> -	build_clear_page();
> -	build_copy_page();
> +	if (smp_processor_id() == 0) {
> +		build_clear_page();
> +		build_copy_page();
> +	}
>   #if !defined(CONFIG_MIPS_CMP)
>   	local_r4k___flush_cache_all(NULL);
>   #endif
> diff --git a/arch/mips/mm/c-tx39.c b/arch/mips/mm/c-tx39.c
> index 87d23ca..1e42e12 100644
> --- a/arch/mips/mm/c-tx39.c
> +++ b/arch/mips/mm/c-tx39.c
> @@ -434,7 +434,9 @@ void __cpuinit tx39_cache_init(void)
>   	printk("Primary data cache %ldkB, linesize %d bytes\n",
>   		dcache_size >> 10, current_cpu_data.dcache.linesz);
>
> -	build_clear_page();
> -	build_copy_page();
> +	if (smp_processor_id() == 0) {
> +		build_clear_page();
> +		build_copy_page();
> +	}
>   	tx39h_flush_icache_all();
>   }
> diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> index a63d1ed..86b4a79 100644
> --- a/arch/mips/mm/tlb-r3k.c
> +++ b/arch/mips/mm/tlb-r3k.c
> @@ -280,5 +280,6 @@ void __cpuinit tlb_init(void)
>   {
>   	local_flush_tlb_all();
>
> -	build_tlb_refill_handler();
> +	if (smp_processor_id() == 0)
> +		build_tlb_refill_handler();
>   }
> diff --git a/arch/mips/mm/tlb-r4k.c b/arch/mips/mm/tlb-r4k.c
> index 0113330..db19624 100644
> --- a/arch/mips/mm/tlb-r4k.c
> +++ b/arch/mips/mm/tlb-r4k.c
> @@ -439,5 +439,6 @@ void __cpuinit tlb_init(void)
>   			printk("Ignoring invalid argument ntlb=%d\n", ntlb);
>   	}
>
> -	build_tlb_refill_handler();
> +	if (smp_processor_id() == 0)
> +		build_tlb_refill_handler();
>   }
> diff --git a/arch/mips/mm/tlb-r8k.c b/arch/mips/mm/tlb-r8k.c
> index 91c2499..43fe634 100644
> --- a/arch/mips/mm/tlb-r8k.c
> +++ b/arch/mips/mm/tlb-r8k.c
> @@ -244,5 +244,6 @@ void __cpuinit tlb_init(void)
>
>   	local_flush_tlb_all();
>
> -	build_tlb_refill_handler();
> +	if (smp_processor_id() == 0)
> +		build_tlb_refill_handler();
>   }
> diff --git a/arch/mips/sgi-ip27/ip27-init.c b/arch/mips/sgi-ip27/ip27-init.c
> index 923c080..62c41ab 100644
> --- a/arch/mips/sgi-ip27/ip27-init.c
> +++ b/arch/mips/sgi-ip27/ip27-init.c
> @@ -84,7 +84,8 @@ static void __cpuinit per_hub_init(cnodeid_t cnode)
>
>   		memcpy((void *)(CKSEG0 + 0x100), &except_vec2_generic, 0x80);
>   		memcpy((void *)(CKSEG0 + 0x180), &except_vec3_generic, 0x80);
> -		build_tlb_refill_handler();
> +		if (smp_processor_id() == 0)
> +			build_tlb_refill_handler();
>   		memcpy((void *)(CKSEG0 + 0x100), (void *) CKSEG0, 0x80);
>   		memcpy((void *)(CKSEG0 + 0x180), &except_vec3_generic, 0x100);
>   		__flush_cache_all();
>