[Devel] Re: [RFC v14-rc][PATCH 18/23] Prepare to support shared memory

[Dave Hansen]

On Fri, 2009-03-20 at 14:47 -0400, Oren Laadan wrote:
> diff --git a/checkpoint/checkpoint_mem.h b/checkpoint/checkpoint_mem.h
> index de1d4c8..3157dde 100644
> --- a/checkpoint/checkpoint_mem.h
> +++ b/checkpoint/checkpoint_mem.h
> @@ -43,4 +43,22 @@ static inline int cr_pgarr_nr_free(struct cr_pgarr *pgarr)
>         return CR_PGARR_TOTAL - pgarr->nr_used;
>  }
> 
> +/*
> + * This probably belongs in include/linux/mm.h
> + */
> +
> +/* Flag allocation requirements to shmem_getpage and shmem_swp_alloc */
> +enum sgp_type {
> +       SGP_READ,       /* don't exceed i_size, don't allocate page */
> +       SGP_CACHE,      /* don't exceed i_size, may allocate page */
> +       SGP_DIRTY,      /* like SGP_CACHE, but set new page dirty */
> +       SGP_WRITE,      /* may exceed i_size, may allocate page */
> +};
> +
> +int shmem_getpage(struct inode *inode, unsigned long idx,
> +                 struct page **pagep, enum sgp_type sgp, int *type);
> +
> +extern struct vm_operations_struct shmem_vm_ops;
> +extern struct vm_operations_struct shm_vm_ops;
...
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -103,8 +103,8 @@ static unsigned long shmem_default_max_inodes(void)
>  }
>  #endif
> 
> -static int shmem_getpage(struct inode *inode, unsigned long idx,
> -                        struct page **pagep, enum sgp_type sgp, int *type);
> +int shmem_getpage(struct inode *inode, unsigned long idx,
> +                 struct page **pagep, enum sgp_type sgp, int *type);

Personally, I think it is bad form to export someone else's function for
them.  Please move the current shmem.c function declaration to a header
that can be included by both the c/r code and shmem.c

> /* return the subtype of a shared vma segment */
> static enum vm_type cr_shared_vma_type(struct vm_area_struct *vma, int new)
> {
>         enum vm_type vma_type;
> 
>         if (vma->vm_ops == &shmem_vm_ops)       /* /dev/zero or anonymous */
>                 vma_type = CR_VMA_SHM_ANON;
>         else if (vma->vm_ops == &shm_vm_ops)    /* IPC (unsupported yet) */
>                 vma_type = -EINVAL;
>         else
>                 vma_type = CR_VMA_SHM_FILE;
> 
>         if (!new && vma_type == CR_VMA_SHM_ANON)
>                 vma_type = CR_VMA_SHM_ANON_SKIP;
> 
>         return vma_type;
> }

This just looks really fragile to me.  The first thing that comes up in
my cscope is bf5xx_pcm_mmap().  That is VM_SHARED, but is *certainly*
not CR_VMA_SHM_FILE.  

Instead of *checking* vma->vm_ops, I believe the more correct solutions
here is to add an new vm_op which can be asked for its CR_VMA_* type.

-- Dave

_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers