Skip to contentSkip to conversation

mm/fs: add execute in place support

PATCH In review Carsten Otte 6 participants 8 messages May 11, 2005

Subsystems (3)
Memory management VFS Page cache
lore
Share & tools
lore

Carsten Otte

author
#0 lore raw

PATCH 2/5 mm/fs: add execute in place support

[RFC/PATCH 2/5] mm/fs: add execute in place support
This patch is the biggest chunk in the patchset. It adds a new address space operation called get_xip_page, which works similar to readpage/writepage but returns a reference to a struct page for the on-disk data for the given page. The page is supposed to be up-to-date. In mm/filemap.c, all generic implementations of file operations are extended to work with the new address space operation if provided.

Signed-off-by Carsten Otte <[email protected]>

---

fs/open.c +3 -1
diff -ruN linux-2.6-git/fs/open.c linux-2.6-git-xip/fs/open.c
@@ -807,7 +807,9 @@
 
 	/* NB: we're sure to have correct a_ops only after f_op->open */
 	if (f->f_flags & O_DIRECT) {
-		if (!f->f_mapping->a_ops || !f->f_mapping->a_ops->direct_IO) {
+		if (!f->f_mapping->a_ops ||
+		    ((!f->f_mapping->a_ops->direct_IO) &&
+		    (!f->f_mapping->a_ops->get_xip_page))) {
 			fput(f);
 			f = ERR_PTR(-EINVAL);
 		}
include/linux/fs.h +24 -8
diff -ruN linux-2.6-git/include/linux/fs.h linux-2.6-git-xip/include/linux/fs.h
@@ -330,6 +330,8 @@
 	int (*releasepage) (struct page *, int);
 	ssize_t (*direct_IO)(int, struct kiocb *, const struct iovec *iov,
 			loff_t offset, unsigned long nr_segs);
+	struct page* (*get_xip_page)(struct address_space *, sector_t,
+			int);
 };
 
 struct backing_dev_info;

@@ -1473,14 +1475,19 @@
 		unsigned long *, loff_t, loff_t *, size_t, size_t);
 extern ssize_t generic_file_buffered_write(struct kiocb *, const struct iovec *,
 		unsigned long, loff_t, loff_t *, size_t, ssize_t);
+extern ssize_t generic_file_xip_write(struct kiocb *, const struct iovec *,
+		unsigned long, loff_t, loff_t *, size_t);
 extern ssize_t do_sync_read(struct file *filp, char __user *buf, size_t len, loff_t *ppos);
 extern ssize_t do_sync_write(struct file *filp, const char __user *buf, size_t len, loff_t *ppos);
 ssize_t generic_file_write_nolock(struct file *file, const struct iovec *iov,
 				unsigned long nr_segs, loff_t *ppos);
 extern ssize_t generic_file_sendfile(struct file *, loff_t *, size_t, read_actor_t, void *);
 extern void do_generic_mapping_read(struct address_space *mapping,
-				    struct file_ra_state *, struct file *,
-				    loff_t *, read_descriptor_t *, read_actor_t);
+				   struct file_ra_state *, struct file *,
+				   loff_t *, read_descriptor_t *, read_actor_t);
+extern void do_xip_mapping_read   (struct address_space *mapping,
+				   struct file_ra_state *, struct file *,
+				   loff_t *, read_descriptor_t *, read_actor_t);
 extern void
 file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);
 extern ssize_t generic_file_direct_IO(int rw, struct kiocb *iocb,

@@ -1494,17 +1501,26 @@
 extern loff_t remote_llseek(struct file *file, loff_t offset, int origin);
 extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
+extern int xip_truncate_page(struct address_space *mapping, loff_t from);
 
 static inline void do_generic_file_read(struct file * filp, loff_t *ppos,
 					read_descriptor_t * desc,
 					read_actor_t actor)
 {
-	do_generic_mapping_read(filp->f_mapping,
-				&filp->f_ra,
-				filp,
-				ppos,
-				desc,
-				actor);
+	if (filp->f_mapping->a_ops->get_xip_page)
+		do_xip_mapping_read(filp->f_mapping,
+					&filp->f_ra,
+					filp,
+					ppos,
+					desc,
+					actor);
+	else
+		do_generic_mapping_read(filp->f_mapping,
+					&filp->f_ra,
+					filp,
+					ppos,
+					desc,
+					actor);
 }
 
 ssize_t __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
mm/filemap.c +384 -3

451 lines not on this page · raw message

#1 lore raw
Quoted 7 lines · Carsten Otte

[RFC/PATCH 2/5] mm/fs: add execute in place support
This patch is the biggest chunk in the patchset. It adds a new address space operation called get_xip_page, which works similar to readpage/writepage but returns a reference to a struct page for the on-disk data for the given page. The page is supposed to be up-to-date. In mm/filemap.c, all generic implementations of file operations are extended to work with the new address space operation if provided.

This is a lot of code for a very special case.

Could you try to put all the xip code into a separate file, e.g. mm/xip.c that's only built when CONFIG_XIP is set? It would probably require duplicating a little more code if you want clean interfaces, e.g. probably a separate set of generic operations.

#2 lore raw
Quoted 9 lines · Christoph Hellwig

This is a lot of code for a very special case.

Could you try to put all the xip code into a separate file, e.g. mm/xip.c that's only built when CONFIG_XIP is set? It would probably require duplicating a little more code if you want clean interfaces, e.g. probably a separate set of generic operations.

Indeed that seems reasonable. There is no exact reason to have this built into a kernel on a platform that does not have a bdev for this. On the other hand, I believe the code should stay in filemap.c, because it fits there conceptually. And I personally dislike #ifdef in the middle of a file.

#3 lore raw
Quoted 11 lines · Carsten Otte
Quoted 9 lines · Christoph Hellwig

This is a lot of code for a very special case.

Could you try to put all the xip code into a separate file, e.g. mm/xip.c that's only built when CONFIG_XIP is set? It would probably require duplicating a little more code if you want clean interfaces, e.g. probably a separate set of generic operations.

Quoted 5 lines

Indeed that seems reasonable. There is no exact reason to have this built into a kernel on a platform that does not have a bdev for this. On the other hand, I believe the code should stay in filemap.c, because it fits there conceptually. And I personally dislike #ifdef in the middle of a file.

While I agree with your reasoning, since you are affecting very hot code path for every architecture, irrespective of "bdev" support for this - you may want to look into some how eliminating few function pointer de-refs and checks for those who don't care. (#ifdef, unlikely(), or some arch & config magic).

To be honest, that file is already complicated enough - every time I look at it my head hurts :(

Thanks,
Badari

#4 lore raw
Quoted 8 lines · Badari Pulavarty

While I agree with your reasoning, since you are affecting very hot

code path for every architecture, irrespective of "bdev" support for this - you may want to look into some how eliminating few function pointer de-refs and checks for those who don't care. (#ifdef, unlikely(), or some arch & config magic).

I do agree that addidional pointer derefs would be a nightmare from the performance perspective. But afaics the patch does not add such, and for checks I did already add likeleyness for the non-xip case. Could you be more precise and specify which code path(es) you mean?

Quoted 4 lines

To be honest, that file is already complicated enough - every time I look at it my head hurts :(

I agree on that one. I gonna put extra functions into its own file.

#5 lore raw
Quoted 11 lines · Carsten Otte
Quoted 5 lines · Badari Pulavarty

you may want to look into some how eliminating few function pointer de-refs and checks for those who don't care. (#ifdef, unlikely(), or some arch & config magic).

I do agree that addidional pointer derefs would be a nightmare from the performance perspective. But afaics the patch does not add such, and for checks I did already add likeleyness for the non-xip case. Could you be more precise and specify which code path(es) you mean?

I guess what Badari means is that you could add a function like

#ifdef CONFIG_FS_XIP
static inline int mapping_has_xip(struct address_space *mapping)

{
	return __unlikely(mapping->a_ops->get_xip_page != NULL);
}

#else
#define mapping_has_xip(x) (0)
#endif

Using this in the hot path should result identical binary code to the current version as long as XIP is not enabled, while otherwise you need to access four data cache lines every time.

I wouldn't expect much benefit from this since all these cache lines should be pretty hot and the branch gets predicted correctly anyway, but it surely doesn't hurt to do the abstraction.

	Arnd <><
#6 lore raw
Quoted 36 lines · Arnd Bergmann
Quoted 10 lines · Carsten Otte
Quoted 7 lines · Badari Pulavarty

you may want to look into some how eliminating few function pointer de-refs and checks for those who don't care. (#ifdef, unlikely(), or some arch & config magic).

Quoted 7 lines

I do agree that addidional pointer derefs would be a nightmare from the performance perspective. But afaics the patch does not add such, and for checks I did already add likeleyness for the non-xip case. Could you be more precise and specify which code path(es) you mean?

I guess what Badari means is that you could add a function like

#ifdef CONFIG_FS_XIP
static inline int mapping_has_xip(struct address_space *mapping)

{
	return __unlikely(mapping->a_ops->get_xip_page != NULL);
}

#else
#define mapping_has_xip(x) (0)
#endif

Using this in the hot path should result identical binary code to the current version as long as XIP is not enabled, while otherwise you need to access four data cache lines every time.

Quoted 7 lines

I wouldn't expect much benefit from this since all these cache lines should be pretty hot and the branch gets predicted correctly anyway, but it surely doesn't hurt to do the abstraction.

	Arnd <><

Agreed. Will be changed in next version, thanks for clarification.

#7 lore raw
In reply to Carsten Otte

Review

Quoted 1 line · Carsten Otte
(quoted diff) +1 -0
+	BUG_ON(!mapping->a_ops->get_xip_page);

No need to put this assert here. You'll get exactly as good a stack trace ...

Review

Quoted 2 lines
(quoted diff) +2 -0
+		page = mapping->a_ops->get_xip_page(mapping,
+			index*(PAGE_SIZE/512), 0);

... here, when you call through a null pointer.

Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain