Skip to contentSkip to conversation

get rid of vm_private_data and win posix shm

PATCH In review Christoph Rohland 2 participants 5 messages Dec 28, 1999 – Dec 31, 1999 1 message not in the archive

Subsystems (8)
Networking Memory management VFS MM core +4 more subsystems
lore
Share & tools
lore

Christoph Rohland

author
#0 lore raw

PATCH 1/1 get rid of vm_private_data and win posix shm

Hi folks,

Here is my first version of using file semantics in shared memory. This obsoletes vm_private_data in vm_area_struct and allows posix shared memory.

I implemented posix shm with its own namespace by extending filp_open and do_unlink by an additional parameter for the root inode.

Also extending this to a complete filesystem should be easy (but not my target).

Greetings
Christoph

P.S. this version is only tested on UP since I cannot access my SMP machine
from home. I will stress test SMP next week.

arch/i386/kernel/entry.S +4 -2
diff -uNr 2.3.34/arch/i386/kernel/entry.S c34/arch/i386/kernel/entry.S
@@ -598,7 +598,9 @@
 	.long SYMBOL_NAME(sys_stat64)		/* 195 */
 	.long SYMBOL_NAME(sys_lstat64)
 	.long SYMBOL_NAME(sys_fstat64)
-
+        .long SYMBOL_NAME(sys_shm_open)
+        .long SYMBOL_NAME(sys_shm_unlink)
+	/* 200 */
 
 	/*
 	 * NOTE!! This doesn't have to be exact - we just have

@@ -606,6 +608,6 @@
 	 * entries. Don't panic if you notice that this hasn't
 	 * been shrunk every time we add a new system call.
 	 */
-	.rept NR_syscalls-197
+	.rept NR_syscalls-199
 		.long SYMBOL_NAME(sys_ni_syscall)
 	.endr
arch/sparc/kernel/sys_sunos.c +0 -1
diff -uNr 2.3.34/arch/sparc/kernel/sys_sunos.c c34/arch/sparc/kernel/sys_sunos.c
@@ -722,7 +722,6 @@
 
 
 extern int do_mount(kdev_t, const char *, const char *, char *, int, void *);
-extern dev_t get_unnamed_dev(void);
 extern void put_unnamed_dev(dev_t);
 extern asmlinkage int sys_mount(char *, char *, char *, unsigned long, void *);
 extern asmlinkage int sys_connect(int fd, struct sockaddr *uservaddr, int addrlen);
arch/sparc64/kernel/sys_sparc32.c +1 -1
diff -uNr 2.3.34/arch/sparc64/kernel/sys_sparc32.c c34/arch/sparc64/kernel/sys_sparc32.c
@@ -2775,7 +2775,7 @@
 	for (i=0 ; i<MAX_ARG_PAGES ; i++)	/* clear page-table */
 		bprm.page[i] = 0;
 
-	dentry = open_namei(filename, 0, 0);
+	dentry = open_namei(filename, 0, 0, NULL);
 	retval = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		return retval;
arch/sparc64/kernel/sys_sunos32.c +0 -1
diff -uNr 2.3.34/arch/sparc64/kernel/sys_sunos32.c c34/arch/sparc64/kernel/sys_sunos32.c
@@ -686,7 +686,6 @@
 };
 
 extern int do_mount(kdev_t, const char *, const char *, char *, int, void *);
-extern dev_t get_unnamed_dev(void);
 extern void put_unnamed_dev(dev_t);
 extern asmlinkage int sys_mount(char *, char *, char *, unsigned long, void *);
 extern asmlinkage int sys_connect(int fd, struct sockaddr *uservaddr, int addrlen);
fs/binfmt_elf.c +2 -2
diff -uNr 2.3.34/fs/binfmt_elf.c c34/fs/binfmt_elf.c
@@ -503,12 +503,12 @@
 					
 				current->personality = PER_SVR4;
 				interpreter_dentry = open_namei(elf_interpreter,
-								0, 0);
+								0, 0, NULL);
 				current->personality = old_pers;
 			} else
 #endif					
 				interpreter_dentry = open_namei(elf_interpreter,
-								0, 0);
+								0, 0, NULL);
 			set_fs(old_fs);
 			retval = PTR_ERR(interpreter_dentry);
 			if (IS_ERR(interpreter_dentry))
fs/binfmt_em86.c +1 -1
diff -uNr 2.3.34/fs/binfmt_em86.c c34/fs/binfmt_em86.c
@@ -79,7 +79,7 @@
 	 * Note that we use open_namei() as the name is now in kernel
 	 * space, and we don't need to copy it.
 	 */
-	dentry = open_namei(interp, 0, 0);
+	dentry = open_namei(interp, 0, 0, NULL);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
fs/binfmt_misc.c +1 -1
diff -uNr 2.3.34/fs/binfmt_misc.c c34/fs/binfmt_misc.c
@@ -214,7 +214,7 @@
 	bprm->argc++;
 	bprm->filename = iname;	/* for binfmt_script */
 
-	dentry = open_namei(iname, 0, 0);
+	dentry = open_namei(iname, 0, 0, NULL);
 	retval = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto _ret;
fs/binfmt_script.c +1 -1
diff -uNr 2.3.34/fs/binfmt_script.c c34/fs/binfmt_script.c
@@ -80,7 +80,7 @@
 	/*
 	 * OK, now restart the process with the interpreter's dentry.
 	 */
-	dentry = open_namei(interp, 0, 0);
+	dentry = open_namei(interp, 0, 0, NULL);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
fs/dquot.c +1 -1
diff -uNr 2.3.34/fs/dquot.c c34/fs/dquot.c
@@ -1486,7 +1486,7 @@
 	if (IS_ERR(tmp))
 		goto out_lock;
 
-	f = filp_open(tmp, O_RDWR, 0600);
+	f = filp_open(tmp, O_RDWR, 0600, NULL);
 	putname(tmp);
 
 	error = PTR_ERR(f);
fs/exec.c +3 -4
diff -uNr 2.3.34/fs/exec.c c34/fs/exec.c
@@ -287,7 +287,6 @@
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = 0;
 		mpnt->vm_file = NULL;
-		mpnt->vm_private_data = (void *) 0;
 		vmlist_modify_lock(current->mm);
 		insert_vm_struct(current->mm, mpnt);
 		vmlist_modify_unlock(current->mm);

@@ -710,7 +709,7 @@
 	        for (i = 0 ; i < MAX_ARG_PAGES ; i++)	/* clear page-table */
                     bprm_loader.page[i] = NULL;
 
-		dentry = open_namei(dynloader[0], 0, 0);
+		dentry = open_namei(dynloader[0], 0, 0, NULL);
 		retval = PTR_ERR(dentry);
 		if (IS_ERR(dentry))
 			return retval;

@@ -775,7 +774,7 @@
 	bprm.p = PAGE_SIZE*MAX_ARG_PAGES-sizeof(void *);
 	memset(bprm.page, 0, MAX_ARG_PAGES*sizeof(bprm.page[0])); 
 
-	dentry = open_namei(filename, 0, 0);
+	dentry = open_namei(filename, 0, 0, NULL);
 	retval = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		return retval;

@@ -855,7 +854,7 @@
 #else
 	corename[4] = '\0';
 #endif
-	file = filp_open(corename, O_CREAT | 2 | O_TRUNC | O_NOFOLLOW, 0600);
+	file = filp_open(corename, O_CREAT | 2 | O_TRUNC | O_NOFOLLOW, 0600, NULL);
 	if (IS_ERR(file))
 		goto fail;
 	dentry = file->f_dentry;
fs/namei.c +5 -5
diff -uNr 2.3.34/fs/namei.c c34/fs/namei.c
@@ -680,7 +680,7 @@
  * which is a lot more logical, and also allows the "no perm" needed
  * for symlinks (where the permissions are checked later).
  */
-struct dentry * open_namei(const char * pathname, int flag, int mode)
+struct dentry * open_namei(const char * pathname, int flag, int mode, struct dentry *root)
 {
 	int acc_mode, error;
 	struct inode *inode;

@@ -689,7 +689,7 @@
 	mode &= S_IALLUGO & ~current->fs->umask;
 	mode |= S_IFREG;
 
-	dentry = lookup_dentry(pathname, NULL, lookup_flags(flag));
+	dentry = lookup_dentry(pathname, root, lookup_flags(flag));
 	if (IS_ERR(dentry))
 		return dentry;
 

@@ -1067,13 +1067,13 @@
 	return error;
 }
 
-static inline int do_unlink(const char * name)
+int do_unlink(const char * name, struct dentry *root)
 {
 	int error;
 	struct dentry *dir;
 	struct dentry *dentry;
 
-	dentry = lookup_dentry(name, NULL, 0);
+	dentry = lookup_dentry(name, root, 0);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto exit;

@@ -1098,7 +1098,7 @@
 	tmp = getname(pathname);
 	error = PTR_ERR(tmp);
 	if (!IS_ERR(tmp)) {
-		error = do_unlink(tmp);
+		error = do_unlink(tmp, NULL);
 		putname(tmp);
 	}
 	unlock_kernel();
fs/open.c +3 -3
diff -uNr 2.3.34/fs/open.c c34/fs/open.c
@@ -656,7 +656,7 @@
  * for the internal routines (ie open_namei()/follow_link() etc). 00 is
  * used by symlinks.
  */
-struct file *filp_open(const char * filename, int flags, int mode)
+struct file *filp_open(const char * filename, int flags, int mode, struct dentry *dent)
 {
 	struct inode * inode;
 	struct dentry * dentry;

@@ -673,7 +673,7 @@
 		flag++;
 	if (flag & O_TRUNC)
 		flag |= 2;
-	dentry = open_namei(filename,flag,mode);
+	dentry = open_namei(filename,flag,mode,dent);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto cleanup_file;

@@ -796,7 +796,7 @@
 		if (fd >= 0) {
 			struct file * f;
 			lock_kernel();
-			f = filp_open(tmp, flags, mode);
+			f = filp_open(tmp, flags, mode, NULL);
 			unlock_kernel();
 			error = PTR_ERR(f);
 			if (IS_ERR(f))
include/asm-i386/unistd.h +2 -0
diff -uNr 2.3.34/include/asm-i386/unistd.h c34/include/asm-i386/unistd.h
@@ -202,6 +202,8 @@
 #define __NR_stat64		195
 #define __NR_lstat64		196
 #define __NR_fstat64		197
+#define __NR_shm_open    	198
+#define __NR_shm_unlink 	199
 
 /* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */
include/linux/fs.h +4 -2
diff -uNr 2.3.34/include/linux/fs.h c34/include/linux/fs.h
@@ -736,7 +736,7 @@
 extern int get_unused_fd(void);
 extern void put_unused_fd(unsigned int);
 
-extern struct file *filp_open(const char *, int, int);
+extern struct file *filp_open(const char *, int, int, struct dentry *);
 extern int filp_close(struct file *, fl_owner_t id);
 
 extern char * getname(const char *);

@@ -840,9 +840,10 @@
 extern int permission(struct inode *, int);
 extern int get_write_access(struct inode *);
 extern void put_write_access(struct inode *);
-extern struct dentry * open_namei(const char *, int, int);
+extern struct dentry * open_namei(const char *, int, int, struct dentry *);
 extern struct dentry * do_mknod(const char *, int, dev_t);
 extern int do_pipe(int *);
+extern int do_unlink(const char * name, struct dentry *root);
 
 /* fs/dcache.c -- generic fs support functions */
 extern int is_subdir(struct dentry *, struct dentry *);

@@ -972,6 +973,7 @@
 extern void put_super(kdev_t);
 unsigned long generate_cluster(kdev_t, int b[], int);
 unsigned long generate_cluster_swab32(kdev_t, int b[], int);
+extern kdev_t get_unnamed_dev(void);
 extern kdev_t ROOT_DEV;
 
 extern void show_buffers(void);
include/linux/mm.h +0 -1
diff -uNr 2.3.34/include/linux/mm.h c34/include/linux/mm.h
@@ -60,7 +60,6 @@
 	struct vm_operations_struct * vm_ops;
 	unsigned long vm_pgoff;		/* offset in PAGE_SIZE units, *not* PAGE_CACHE_SIZE */
 	struct file * vm_file;
-	void * vm_private_data;		/* was vm_pte (shared mem) */
 };
 
 /*
ipc/shm.c +424 -267

958 lines not on this page · raw message

kernel/acct.c +1 -1
diff -uNr 2.3.34/kernel/acct.c c34/kernel/acct.c
@@ -162,7 +162,7 @@
 		if (IS_ERR(tmp))
 			goto out;
 		/* Difference from BSD - they don't do O_APPEND */
-		file = filp_open(tmp, O_WRONLY|O_APPEND, 0);
+		file = filp_open(tmp, O_WRONLY|O_APPEND, 0, NULL);
 		putname(tmp);
 		if (IS_ERR(file)) {
 			error = PTR_ERR(file);
mm/mmap.c +0 -4
diff -uNr 2.3.34/mm/mmap.c c34/mm/mmap.c
@@ -271,7 +271,6 @@
 	vma->vm_ops = NULL;
 	vma->vm_pgoff = pgoff;
 	vma->vm_file = NULL;
-	vma->vm_private_data = NULL;
 
 	/* Clear old maps */
 	error = -ENOMEM;

@@ -547,7 +546,6 @@
 		mpnt->vm_ops = area->vm_ops;
 		mpnt->vm_pgoff = area->vm_pgoff + ((end - area->vm_start) >> PAGE_SHIFT);
 		mpnt->vm_file = area->vm_file;
-		mpnt->vm_private_data = area->vm_private_data;
 		if (mpnt->vm_file)
 			get_file(mpnt->vm_file);
 		if (mpnt->vm_ops && mpnt->vm_ops->open)

@@ -780,7 +778,6 @@
 	vma->vm_ops = NULL;
 	vma->vm_pgoff = 0;
 	vma->vm_file = NULL;
-	vma->vm_private_data = NULL;
 
 	/*
 	 * merge_segments may merge our vma, so we can't refer to it

@@ -927,7 +924,6 @@
 
 		/* To share, we must have the same file, operations.. */
 		if ((mpnt->vm_file != prev->vm_file)||
-		    (mpnt->vm_private_data != prev->vm_private_data)	||
 		    (mpnt->vm_ops != prev->vm_ops)	||
 		    (mpnt->vm_flags != prev->vm_flags)	||
 		    (prev->vm_end != mpnt->vm_start))
net/khttpd/security.c +1 -1
diff -uNr 2.3.34/net/khttpd/security.c c34/net/khttpd/security.c
@@ -115,7 +115,7 @@
 
 	
 		
-	filp = filp_open(Filename,00,O_RDONLY);
+	filp = filp_open(Filename,00,O_RDONLY,NULL);
 	
 	
 	if ((IS_ERR(filp))||(filp==NULL)||(filp->f_dentry==NULL))
net/unix/af_unix.c +1 -1
diff -uNr 2.3.34/net/unix/af_unix.c c34/net/unix/af_unix.c
@@ -572,7 +572,7 @@
 
 		/* Do not believe to VFS, grab kernel lock */
 		lock_kernel();
-		dentry = open_namei(sunname->sun_path, 2|O_NOFOLLOW, S_IFSOCK);
+		dentry = open_namei(sunname->sun_path, 2|O_NOFOLLOW, S_IFSOCK, NULL);
 		if (IS_ERR(dentry)) {
 			*error = PTR_ERR(dentry);
 			unlock_kernel();
#1 lore raw
Quoted 4 lines

I implemented posix shm with its own namespace by extending filp_open and do_unlink by an additional parameter for the root inode. Also extending this to a complete filesystem should be easy (but not my target).

It would seem that the best way to fix the inelegance of the patch - the shm_open and shm_unlink syscalls, the hacks on filp_open etc would be to do exactly that - make it a real fs, at least for open/unlink/openddir/readdir even if not for read/write

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.nl.linux.org/Linux-MM/

author
#2 lore raw
Quoted 9 lines · Alan Cox
Quoted 4 lines

I implemented posix shm with its own namespace by extending filp_open and do_unlink by an additional parameter for the root inode. Also extending this to a complete filesystem should be easy (but not my target).

It would seem that the best way to fix the inelegance of the patch - the shm_open and shm_unlink syscalls, the hacks on filp_open etc would be to do exactly that - make it a real fs, at least for open/unlink/openddir/readdir even if not for read/write

This makes the sysv ipc code dependent on a mounted fs. Also the library has to know where this shm fs is mounted to implement shm_open etc. I do not like these ideas. But I know this is questionable.

I will redo my patch to be much less intrusive into other code.

Greetings
Christoph

Signature · 4 lines
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.nl.linux.org/Linux-MM/
author
#4 lore raw
In reply to a message not in the archive
Quoted 6 lines · Linus Torvalds

In article <[email protected]>,

Quoted 3 lines · Christoph Rohland

I implemented posix shm with its own namespace by extending filp_open and do_unlink by an additional parameter for the root inode.

Quoted 8 lines

Beautiful patch _except_ for this case. I'm really pleased with how well the POSIX shm code seems to integrate into the FS and VM layers, and that makes me happy.

The one imbalance you added makes me cringe, though. I think we should just export it as a real filesystem, and mount it in a standard location. Nothing clever, just come up with a new location that is fixed and acceptable to all, kind of like /proc is now.

O.K. After your and Alans objections I probably have to get rid of my separate namespace ;-(

How do I do the SYSV shm stuff then? On creation I could grab the first superblock and create the object there. But on removal I rely on the fs unlink function through do_unlink. How do I get the right path for the plain unlink call? You do not propose to code the location into the kernel, don't you?

Any advice welcome
Christoph
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.nl.linux.org/Linux-MM/

author
#5 lore raw

PATCH 1/1 get rid of vm_private_data and win posix shm version 2

In reply to Christoph Rohland

Hi folks,

This new patch now avoids the new syscalls and the separate namespace by implementing a minimal filesystem. I have to admit that this simplified the code a lot. But keep in mind that you now have to mount the fs to be able to use SYSV SHM.

Also you can mount the fs only once. This will probably break shm in chrooted environments.

It works fine for me on UP. SMP is still not tested.

Greetings
Christoph

fs/exec.c +0 -1
diff -uNr 2.3.34/fs/exec.c c34/fs/exec.c
@@ -287,7 +287,6 @@
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = 0;
 		mpnt->vm_file = NULL;
-		mpnt->vm_private_data = (void *) 0;
 		vmlist_modify_lock(current->mm);
 		insert_vm_struct(current->mm, mpnt);
 		vmlist_modify_unlock(current->mm);
fs/namei.c +1 -1
diff -uNr 2.3.34/fs/namei.c c34/fs/namei.c
@@ -1067,7 +1067,7 @@
 	return error;
 }
 
-static inline int do_unlink(const char * name)
+int do_unlink(const char * name)
 {
 	int error;
 	struct dentry *dir;
include/linux/fs.h +1 -0
diff -uNr 2.3.34/include/linux/fs.h c34/include/linux/fs.h
@@ -843,6 +843,7 @@
 extern struct dentry * open_namei(const char *, int, int);
 extern struct dentry * do_mknod(const char *, int, dev_t);
 extern int do_pipe(int *);
+extern int do_unlink(const char * name);
 
 /* fs/dcache.c -- generic fs support functions */
 extern int is_subdir(struct dentry *, struct dentry *);
include/linux/mm.h +0 -1
diff -uNr 2.3.34/include/linux/mm.h c34/include/linux/mm.h
@@ -60,7 +60,6 @@
 	struct vm_operations_struct * vm_ops;
 	unsigned long vm_pgoff;		/* offset in PAGE_SIZE units, *not* PAGE_CACHE_SIZE */
 	struct file * vm_file;
-	void * vm_private_data;		/* was vm_pte (shared mem) */
 };
 
 /*
ipc/shm.c +539 -294

1,118 lines not on this page · raw message

mm/mmap.c +0 -4
diff -uNr 2.3.34/mm/mmap.c c34/mm/mmap.c
@@ -271,7 +271,6 @@
 	vma->vm_ops = NULL;
 	vma->vm_pgoff = pgoff;
 	vma->vm_file = NULL;
-	vma->vm_private_data = NULL;
 
 	/* Clear old maps */
 	error = -ENOMEM;

@@ -547,7 +546,6 @@
 		mpnt->vm_ops = area->vm_ops;
 		mpnt->vm_pgoff = area->vm_pgoff + ((end - area->vm_start) >> PAGE_SHIFT);
 		mpnt->vm_file = area->vm_file;
-		mpnt->vm_private_data = area->vm_private_data;
 		if (mpnt->vm_file)
 			get_file(mpnt->vm_file);
 		if (mpnt->vm_ops && mpnt->vm_ops->open)

@@ -780,7 +778,6 @@
 	vma->vm_ops = NULL;
 	vma->vm_pgoff = 0;
 	vma->vm_file = NULL;
-	vma->vm_private_data = NULL;
 
 	/*
 	 * merge_segments may merge our vma, so we can't refer to it

@@ -927,7 +924,6 @@
 
 		/* To share, we must have the same file, operations.. */
 		if ((mpnt->vm_file != prev->vm_file)||
-		    (mpnt->vm_private_data != prev->vm_private_data)	||
 		    (mpnt->vm_ops != prev->vm_ops)	||
 		    (mpnt->vm_flags != prev->vm_flags)	||
 		    (prev->vm_end != mpnt->vm_start))