Re: [patch 10/21] Xen-paravirt: add hooks to intercept mm creation and destruction

Andrew Morton
2007-02-16T06:35:12Z

On Thu, 15 Feb 2007 18:24:59 -0800 Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Add hooks to allow a paravirt implementation to track the lifetime of
> an mm.
>
> --- a/arch/i386/kernel/paravirt.c
> +++ b/arch/i386/kernel/paravirt.c
> @@ -706,6 +706,10 @@ struct paravirt_ops paravirt_ops = {
> .irq_enable_sysexit = native_irq_enable_sysexit,
> .iret = native_iret,
>
> + .dup_mmap = (void *)native_nop,
> + .exit_mmap = (void *)native_nop,
> + .activate_mm = (void *)native_nop,
> +
> .startup_ipi_hook = (void *)native_nop,
> };

eww. I suppose there's a good reason for the casting.

> ===================================================================
> --- a/include/asm-i386/mmu_context.h
> +++ b/include/asm-i386/mmu_context.h
> @@ -5,6 +5,7 @@
> #include <asm/atomic.h>
> #include <asm/pgalloc.h>
> #include <asm/tlbflush.h>
> +#include <asm/paravirt.h>
>
> /*
> * Used for LDT copy/destruction.
> @@ -65,7 +66,10 @@ static inline void switch_mm(struct mm_s
> #define deactivate_mm(tsk, mm) \
> asm("movl %0,%%gs": :"r" (0));
>
> -#define activate_mm(prev, next) \
> - switch_mm((prev),(next),NULL)
> +#define activate_mm(prev, next) \
> + do { \
> + arch_activate_mm(prev, next); \
> + switch_mm((prev),(next),NULL); \
> + } while(0);

It seems strange to call out to arch_foo() from within an arch header file.
I mean, we implicity know we're i386.

Maybe it's just poorly named.

> +static inline void paravirt_activate_mm(struct mm_struct *prev,
> + struct mm_struct *next)
> +{
> +}
> +
> +static inline void paravirt_dup_mmap(struct mm_struct *oldmm,
> + struct mm_struct *mm)
> +{
> +}
> +
> +static inline void paravirt_exit_mmap(struct mm_struct *mm)
> +{
> +}

These functions are unreferenced in this patchset.

> #endif /* CONFIG_PARAVIRT */
> #endif /* __ASM_PARAVIRT_H */
> ===================================================================
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -374,6 +374,12 @@ struct mm_struct {
> rwlock_t ioctx_list_lock;
> struct kioctx *ioctx_list;
> };
> +
> +#ifndef __HAVE_ARCH_MM_LIFETIME
> +#define arch_activate_mm(prev, next) do {} while(0)
> +#define arch_dup_mmap(oldmm, mm) do {} while(0)
> +#define arch_exit_mmap(mm) do {} while(0)
> +#endif

Can we lose __HAVE_ARCH_MM_LIFETIME? Just define these (preferably in C,
not in cpp) in the appropriate include/asm-foo/ files?

> struct sighand_struct {
> atomic_t count;
> ===================================================================
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -286,6 +286,7 @@ static inline int dup_mmap(struct mm_str
> if (retval)
> goto out;
> }
> + arch_dup_mmap(oldmm, mm);
> retval = 0;
> out:
> up_write(&mm->mmap_sem);
> ===================================================================
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1976,6 +1976,8 @@ void exit_mmap(struct mm_struct *mm)
> struct vm_area_struct *vma = mm->mmap;
> unsigned long nr_accounted = 0;
> unsigned long end;
> +
> + arch_exit_mmap(mm);
>
> lru_add_drain();
> flush_cache_mm(mm);

Perhaps some commentary telling the arch maintainer what these hooks he's
being offered are for?