Re: [RFC][PATCH] Anonymous shared memory (ashmem) subsystem

[Arnd Bergmann <arnd@arndb.de>]

On Monday 18 July 2011, John Stultz wrote:

> +
> +#define ASHMEM_NAME_DEF		"dev/ashmem"
> +
> +/* Return values from ASHMEM_PIN: Was the mapping purged while unpinned? */
> +#define ASHMEM_NOT_PURGED	0
> +#define ASHMEM_WAS_PURGED	1
> +
> +/* Return values from ASHMEM_GET_PIN_STATUS: Is the mapping pinned? */
> +#define ASHMEM_IS_UNPINNED	0
> +#define ASHMEM_IS_PINNED	1
> +
> +struct ashmem_pin {
> +	__u32 offset;	/* offset into region, in bytes, page-aligned */
> +	__u32 len;	/* length forward from offset, in bytes, page-aligned */
> +};
> +
> +#define __ASHMEMIOC		0x77
> +
> +#define ASHMEM_SET_NAME		_IOW(__ASHMEMIOC, 1, char[ASHMEM_NAME_LEN])
> +#define ASHMEM_GET_NAME		_IOR(__ASHMEMIOC, 2, char[ASHMEM_NAME_LEN])

Having a name for an 'anonymous shared memory' segment is rather
counter-intuitive ;-)

> +#define ASHMEM_SET_SIZE		_IOW(__ASHMEMIOC, 3, size_t)
> +#define ASHMEM_GET_SIZE		_IO(__ASHMEMIOC, 4)
> +#define ASHMEM_SET_PROT_MASK	_IOW(__ASHMEMIOC, 5, unsigned long)
> +#define ASHMEM_GET_PROT_MASK	_IO(__ASHMEMIOC, 6)

size_t and unsigned long arguments in ioctl commands are harmful,
because they require a compat_ioctl function. It's often better to just
make these either __u32 or __u64.

> diff --git a/mm/Makefile b/mm/Makefile
> index 836e416..cd41f09 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_HUGETLBFS)	+= hugetlb.o
>  obj-$(CONFIG_NUMA) 	+= mempolicy.o
>  obj-$(CONFIG_SPARSEMEM)	+= sparse.o
>  obj-$(CONFIG_SPARSEMEM_VMEMMAP) += sparse-vmemmap.o
> +obj-$(CONFIG_ASHMEM)	+= ashmem.o
>  obj-$(CONFIG_SLOB) += slob.o
>  obj-$(CONFIG_COMPACTION) += compaction.o
>  obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o

Wouldn't this better live in drivers/char, next to mem.c or even
merged into that file?

> +static const struct file_operations ashmem_fops = {
> +	.owner = THIS_MODULE,
> +	.open = ashmem_open,
> +	.release = ashmem_release,
> +	.read = ashmem_read,
> +	.llseek = ashmem_llseek,
> +	.mmap = ashmem_mmap,
> +	.unlocked_ioctl = ashmem_ioctl,
> +	.compat_ioctl = ashmem_ioctl,
> +};

The compat_ioctl is wrong here, as described above.

	Arnd