Re: [patch 07/16] x86: pvclock: generic pvclock vsyscall initialization

[Glauber Costa <glommer@parallels.com>]

On 11/01/2012 02:47 AM, Marcelo Tosatti wrote:
> Originally from Jeremy Fitzhardinge.
> 
> Introduce generic, non hypervisor specific, pvclock initialization 
> routines.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: vsyscall/arch/x86/kernel/pvclock.c
> ===================================================================
> --- vsyscall.orig/arch/x86/kernel/pvclock.c
> +++ vsyscall/arch/x86/kernel/pvclock.c
> @@ -17,6 +17,10 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/percpu.h>
> +#include <linux/notifier.h>
> +#include <linux/sched.h>
> +#include <linux/gfp.h>
> +#include <linux/bootmem.h>
>  #include <asm/pvclock.h>
>  
>  static u8 valid_flags __read_mostly = 0;
> @@ -122,3 +126,67 @@ void pvclock_read_wallclock(struct pvclo
>  
>  	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
>  }
> +
> +static aligned_pvti_t *pvclock_vdso_info;
is there a non-aligned pvti ?

I would also add that "pvti" is not exactly the most descriptive name
I've seen.

> +
> +static struct pvclock_vsyscall_time_info *
> +pvclock_get_vsyscall_user_time_info(int cpu)
> +{
> +	if (pvclock_vdso_info == NULL) {
> +		BUG();
> +		return NULL;
> +	}
> +
> +	return &pvclock_vdso_info[cpu].info;
> +}
> +
if (!pvclock_vdso_info)
    BUG();
return &pvclock...

> +struct pvclock_vcpu_time_info *pvclock_get_vsyscall_time_info(int cpu)
> +{
> +	return &pvclock_get_vsyscall_user_time_info(cpu)->pvti;
> +}
> +
> +int pvclock_task_migrate(struct notifier_block *nb, unsigned long l, void *v)
> +{
> +	struct task_migration_notifier *mn = v;
> +	struct pvclock_vsyscall_time_info *pvti;
> +
> +	pvti = pvclock_get_vsyscall_user_time_info(mn->from_cpu);
> +
> +	if (pvti == NULL)
> +		return NOTIFY_DONE;
> +

When is it NULL? IIUC, this is when the vsyscall is disabled, right?
Would you mind adding comments for it?

Also, this is supposed to be an unlikely branch, right?

> +	pvti->migrate_count++;
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block pvclock_migrate = {
> +	.notifier_call = pvclock_task_migrate,
> +};
> +
> +/*
> + * Initialize the generic pvclock vsyscall state.  This will allocate
> + * a/some page(s) for the per-vcpu pvclock information, set up a
> + * fixmap mapping for the page(s)
> + */
> +int __init pvclock_init_vsyscall(void)
> +{
> +	int idx;
> +	unsigned int size = PVCLOCK_VSYSCALL_NR_PAGES*PAGE_SIZE;
> +
> +	pvclock_vdso_info = __alloc_bootmem(size, PAGE_SIZE, 0);

This will be the most critical path for reading pvclock's time data. So
why can't we:
1) Make it per-node.
2) Of course, as a consequence, make sure all info structures in the
same page are in the same node ?



> +	if (!pvclock_vdso_info)
> +		return -ENOMEM;
> +
> +	memset(pvclock_vdso_info, 0, size);
> +
> +	for (idx = 0; idx <= (PVCLOCK_FIXMAP_END-PVCLOCK_FIXMAP_BEGIN); idx++) {
> +		__set_fixmap(PVCLOCK_FIXMAP_BEGIN + idx,
> +			     __pa_symbol(pvclock_vdso_info) + (idx*PAGE_SIZE),
> +			     PAGE_KERNEL_VVAR);
> +	}
> +
> +	register_task_migration_notifier(&pvclock_migrate);
> +
> +	return 0;
> +}
> Index: vsyscall/arch/x86/include/asm/fixmap.h
> ===================================================================
> --- vsyscall.orig/arch/x86/include/asm/fixmap.h
> +++ vsyscall/arch/x86/include/asm/fixmap.h
> @@ -19,6 +19,7 @@
>  #include <asm/acpi.h>
>  #include <asm/apicdef.h>
>  #include <asm/page.h>
> +#include <asm/pvclock.h>
>  #ifdef CONFIG_X86_32
>  #include <linux/threads.h>
>  #include <asm/kmap_types.h>
> @@ -81,6 +82,10 @@ enum fixed_addresses {
>  	VVAR_PAGE,
>  	VSYSCALL_HPET,
>  #endif
> +#ifdef CONFIG_PARAVIRT_CLOCK
> +	PVCLOCK_FIXMAP_BEGIN,
> +	PVCLOCK_FIXMAP_END = PVCLOCK_FIXMAP_BEGIN+PVCLOCK_VSYSCALL_NR_PAGES-1,
> +#endif
>  	FIX_DBGP_BASE,
>  	FIX_EARLYCON_MEM_BASE,
>  #ifdef CONFIG_PROVIDE_OHCI1394_DMA_INIT
> Index: vsyscall/arch/x86/include/asm/pvclock.h
> ===================================================================
> --- vsyscall.orig/arch/x86/include/asm/pvclock.h
> +++ vsyscall/arch/x86/include/asm/pvclock.h
> @@ -13,6 +13,8 @@ void pvclock_read_wallclock(struct pvclo
>  			    struct pvclock_vcpu_time_info *vcpu,
>  			    struct timespec *ts);
>  void pvclock_resume(void);
> +int __init pvclock_init_vsyscall(void);
> +struct pvclock_vcpu_time_info *pvclock_get_vsyscall_time_info(int cpu);
>  
>  /*
>   * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
> @@ -85,4 +87,17 @@ unsigned __pvclock_read_cycles(const str
>  	return version;
>  }
>  
> +struct pvclock_vsyscall_time_info {
> +	struct pvclock_vcpu_time_info pvti;
> +	u32 migrate_count;
> +};
> +
> +typedef union {
> +	struct pvclock_vsyscall_time_info info;
> +	char pad[SMP_CACHE_BYTES];
> +} aligned_pvti_t ____cacheline_aligned;

Please, just align pvclock_vsyscall_time_info. It is 10 thousand times
more descriptive.