[Devel] Re: [PATCH 09/12] Move mmap checkpoint/restart into mm/mmap.c

[Nathan Lynch]

On Fri, 2010-02-26 at 00:45 -0800, Matt Helsley wrote:
> Moving the memory pieces is more complicated because portions of it are
> shared with ipc. Split the mm header bits into another
> header file: include/linux/mm_checkpoint.h file so it's clear that these
> are the only pieces we need in ipc/

> diff --git a/include/linux/mm_checkpoint.h b/include/linux/mm_checkpoint.h
> new file mode 100644
> index 0000000..0092321
> --- /dev/null
> +++ b/include/linux/mm_checkpoint.h
> @@ -0,0 +1,45 @@
> +#ifndef _LINUX_MM_CHECKPOINT_H
> +#define _LINUX_MM_CHECKPOINT_H
> +
> +#include <linux/checkpoint.h> /* for ckpt_obj_fetch, restore_read_page */

But ckpt_obj_fetch and restore_read_page aren't referred to in this
file?  I understand that users of mm_checkpoint.h may need those APIs
but it's more conventional for those users to include the appropriate
headers directly.


> +#include <linux/checkpoint_hdr.h> /* for struct ckpt_hdr_vma */
> +#include <linux/checkpoint_types.h> /* for struct ckpt_ctx */

You can just declare these like so:

struct ckpt_hdr_vma;
struct ckpt_ctx;

Need similar for inode, vm_area_struct, etc. -- I think so far you're
just getting lucky because checkpoint headers are included late and the
headers for the objects they refer to have already been included.


> +
> +extern void ckpt_pgarr_free(struct ckpt_ctx *ctx);
> +
> +extern int checkpoint_obj_mm(struct ckpt_ctx *ctx, struct task_struct *t);
> +extern int restore_obj_mm(struct ckpt_ctx *ctx, int mm_objref);
> +
> +extern int ckpt_collect_mm(struct ckpt_ctx *ctx, struct task_struct *t);
> +
> +extern int checkpoint_memory_contents(struct ckpt_ctx *ctx,
> +				      struct vm_area_struct *vma,
> +				      struct inode *inode);
> +extern int restore_memory_contents(struct ckpt_ctx *ctx, struct inode *inode);
> +
> +/* common vma checkpoint/restore operations */
> +extern int generic_vma_checkpoint(struct ckpt_ctx *ctx,
> +				  struct vm_area_struct *vma,
> +				  enum vma_type type,
> +				  int vma_objref, int ino_objref);
> +extern unsigned long generic_vma_restore(struct mm_struct *mm,
> +					 struct file *file,
> +					 struct ckpt_hdr_vma *h);
> +extern int private_vma_checkpoint(struct ckpt_ctx *ctx,
> +				  struct vm_area_struct *vma,
> +				  enum vma_type type,
> +				  int vma_objref);
> +extern int private_vma_restore(struct ckpt_ctx *ctx, struct mm_struct *mm,
> +			       struct file *file, struct ckpt_hdr_vma *h);
> +extern int shmem_vma_checkpoint(struct ckpt_ctx *ctx,
> +				struct vm_area_struct *vma,
> +				enum vma_type type,
> +				int ino_objref);
> +
> +
> +#define CKPT_VMA_NOT_SUPPORTED						\
> +	(VM_IO | VM_HUGETLB | VM_NONLINEAR | VM_PFNMAP |		\
> +	 VM_RESERVED | VM_NORESERVE | VM_HUGETLB | VM_NONLINEAR |	\
> +	 VM_MAPPED_COPY | VM_INSERTPAGE | VM_MIXEDMAP | VM_SAO)
> +
> +#endif /* _LINUX_MM_CHECKPOINT_H */


> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 379eaed..e187078 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -173,12 +173,6 @@ extern void proc_net_remove(struct net *net, const char *name);
>  extern struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
>  	struct proc_dir_entry *parent);
>  
> -/* While the {get|set|dup}_mm_exe_file functions are for mm_structs, they are
> - * only needed to implement /proc/<pid>|self/exe so we define them here. */
> -extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
> -extern struct file *get_mm_exe_file(struct mm_struct *mm);
> -extern void dup_mm_exe_file(struct mm_struct *oldmm, struct mm_struct *newmm);
> -
>  #else
>  
>  #define proc_net_fops_create(net, name, mode, fops)  ({ (void)(mode), NULL; })
> @@ -226,19 +220,6 @@ static inline void pid_ns_release_proc(struct pid_namespace *ns)
>  {
>  }
>  
> -static inline void set_mm_exe_file(struct mm_struct *mm,
> -				   struct file *new_exe_file)
> -{}
> -
> -static inline struct file *get_mm_exe_file(struct mm_struct *mm)
> -{
> -	return NULL;
> -}
> -
> -static inline void dup_mm_exe_file(struct mm_struct *oldmm,
> -	       			   struct mm_struct *newmm)
> -{}
> -
>  #endif /* CONFIG_PROC_FS */
>  
>  #if !defined(CONFIG_PROC_KCORE)
> diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
> index 4322dea..2b05067 100644
> --- a/ipc/checkpoint.c
> +++ b/ipc/checkpoint.c
> @@ -15,8 +15,7 @@
>  #include <linux/msg.h>
>  #include <linux/sched.h>
>  #include <linux/ipc_namespace.h>
> -#include <linux/checkpoint.h>
> -#include <linux/checkpoint_hdr.h>
> +#include <linux/mm_checkpoint.h>

I'm not sure I understand the reason for this kind of change in this
patch.  ipc/checkpoint.c surely uses the APIs in checkpoint.h and
checkpoint_hdr.h so it should include them directly -- that's the usual
Linux practice AFAIK.  By all means, if there's a foo.c that needs only
the APIs in mm_checkpoint.h, then it should include only that.


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