Dave Hansen wrote:
> 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.
Yes, it will be cleaner. I'll give it a try.
Oren
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
