Reoganize data elements to reduce memory footprint

PATCH New Goldwyn Rodrigues 3 participants 7 messages Jun 9, 2010 – Jun 10, 2010 OCFS2

Share & tools

Goldwyn Rodrigues

author

2010-06-09 21:57 UTC #0 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

This is the re-arrangement of the data elements of ocfs2 data structures to reduce memory consumption as shown by pahole on an x86_64 box. I have tried to keep the context as close as possible, though I was pretty agressive to get the numbers down.

Statistics in bytes: (before - after = reduction)
ocfs2_write_ctxt: 2144 - 2136 = 8
ocfs2_inode_info: 1960 - 1896 = 64
ocfs2_journal: 168 - 160 = 8
ocfs2_lock_res: 336 - 320 = 16
ocfs2_refcount_tree: 512 - 488 = 24

Signed-off-by Goldwyn Rodrigues <[email protected]>

---

fs/ocfs2/aops.c +1 -1

index 3623ca2..1b5e284 100644
@@ -910,8 +910,8 @@ struct ocfs2_write_ctxt {
 	 * out in so that future reads from that region will get
 	 * zero's.
 	 */
-	struct page			*w_pages[OCFS2_MAX_CTXT_PAGES];
 	unsigned int			w_num_pages;
+	struct page			*w_pages[OCFS2_MAX_CTXT_PAGES];
 	struct page			*w_target_page;
 
 	/*

fs/ocfs2/inode.h +3 -8

index 9f5f5fc..e2b0053 100644
@@ -46,26 +46,21 @@ struct ocfs2_inode_info
 	/* These fields are protected by ip_lock */
 	spinlock_t			ip_lock;
 	u32				ip_open_count;
-	u32				ip_clusters;
 	struct list_head		ip_io_markers;
+	u32				ip_clusters;
 
+	u16				ip_dyn_features;
 	struct mutex			ip_io_mutex;
-
 	u32				ip_flags; /* see below */
 	u32				ip_attr; /* inode attributes */
-	u16				ip_dyn_features;
 
 	/* protected by recovery_lock. */
 	struct inode			*ip_next_orphan;
-
-	u32				ip_dir_start_lookup;
-
 	struct ocfs2_caching_info	ip_metadata_cache;
-
 	struct ocfs2_extent_map		ip_extent_map;
-
 	struct inode			vfs_inode;
 	struct jbd2_inode		ip_jinode;
+	u32				ip_dir_start_lookup;
 
 	/* Only valid if the inode is the dir. */
 	u32				ip_last_used_slot;

fs/ocfs2/journal.h +1 -1

index b5baaa8..ed05ac3 100644
@@ -67,11 +67,11 @@ struct ocfs2_journal {
 	struct buffer_head        *j_bh;      /* Journal disk inode block */
 	atomic_t                  j_num_trans; /* Number of transactions
 					        * currently in the system. */
+	spinlock_t                j_lock;
 	unsigned long             j_trans_id;
 	struct rw_semaphore       j_trans_barrier;
 	wait_queue_head_t         j_checkpointed;
 
-	spinlock_t                j_lock;
 	struct list_head          j_la_cleanups;
 	struct work_struct        j_recovery_work;
 };

fs/ocfs2/ocfs2.h +2 -2

index c67003b..34b9c79 100644
@@ -151,17 +151,16 @@ struct ocfs2_lock_res {
 	void                    *l_priv;
 	struct ocfs2_lock_res_ops *l_ops;
 	spinlock_t               l_lock;
+	enum ocfs2_lock_type     l_type;
 
 	struct list_head         l_blocked_list;
 	struct list_head         l_mask_waiters;
 
-	enum ocfs2_lock_type     l_type;
 	unsigned long		 l_flags;
 	char                     l_name[OCFS2_LOCK_ID_MAX_LEN];
 	int                      l_level;
 	unsigned int             l_ro_holders;
 	unsigned int             l_ex_holders;
-	struct ocfs2_dlm_lksb    l_lksb;
 
 	/* used from AST/BAST funcs. */
 	enum ocfs2_ast_action    l_action;

@@ -170,6 +169,7 @@ struct ocfs2_lock_res {
 	int                      l_blocking;
 	unsigned int             l_pending_gen;
 
+	struct ocfs2_dlm_lksb    l_lksb;
 	wait_queue_head_t        l_event;
 
 	struct list_head         l_debug_list;

fs/ocfs2/refcounttree.h +2 -2

index 9983ba1..67a89e4 100644
@@ -21,14 +21,14 @@ struct ocfs2_refcount_tree {
 	struct rb_node rf_node;
 	u64 rf_blkno;
 	u32 rf_generation;
+	struct kref rf_getcnt;
 	struct rw_semaphore rf_sem;
 	struct ocfs2_lock_res rf_lockres;
-	struct kref rf_getcnt;
 	int rf_removed;
+	spinlock_t rf_lock;
 
 	/* the following 4 fields are used by caching_info. */
 	struct ocfs2_caching_info rf_ci;
-	spinlock_t rf_lock;
 	struct mutex rf_io_mutex;
 	struct super_block *rf_sb;
 };

Signature · 1 line

Goldwyn

Sunil Mushran

2010-06-09 23:46 UTC #1 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

comments inlined.

Quoted 19 lines · Goldwyn Rodrigues

This is the re-arrangement of the data elements of ocfs2 data structures to reduce memory consumption as shown by pahole on an x86_64 box. I have tried to keep the context as close as possible, though I was pretty agressive to get the numbers down.
Statistics in bytes: (before - after = reduction)
ocfs2_write_ctxt: 2144 - 2136 = 8
ocfs2_inode_info: 1960 - 1896 = 64
ocfs2_journal: 168 - 160 = 8
ocfs2_lock_res: 336 - 320 = 16
ocfs2_refcount_tree: 512 - 488 = 24
Signed-off-by Goldwyn Rodrigues<[email protected]>
---
fs/ocfs2/aops.c +0 -0
index 3623ca2..1b5e284 100644

fs/ocfs2/aops.c struct ocfs2_write_ctxt { · line 916

Quoted 8 lines

fs/ocfs2/aops.c +1 -1

@@ -910,8 +910,8 @@ struct ocfs2_write_ctxt {
  	 * out in so that future reads from that region will get
  	 * zero's.
  	 */
-	struct page			*w_pages[OCFS2_MAX_CTXT_PAGES];
  	unsigned int			w_num_pages;
+	struct page			*w_pages[OCFS2_MAX_CTXT_PAGES];
  	struct page			*w_target_page;

looks good.

fs/ocfs2/inode.h struct ocfs2_inode_info · line 51

Quoted 13 lines

fs/ocfs2/aops.c +0 -0

/*

fs/ocfs2/inode.h +1 -1

index 9f5f5fc..e2b0053 100644
@@ -46,26 +46,21 @@ struct ocfs2_inode_info
  	/* These fields are protected by ip_lock */
  	spinlock_t			ip_lock;
  	u32				ip_open_count;
-	u32				ip_clusters;
  	struct list_head		ip_io_markers;
+	u32				ip_clusters;

ok.

fs/ocfs2/inode.h struct ocfs2_inode_info · line 56

Quoted 7 lines

fs/ocfs2/inode.h +1 -2

+	u16				ip_dyn_features;
  	struct mutex			ip_io_mutex;
-
  	u32				ip_flags; /* see below */
  	u32				ip_attr; /* inode attributes */
-	u16				ip_dyn_features;

ok.

fs/ocfs2/inode.h struct ocfs2_inode_info · line 64

Quoted 13 lines

fs/ocfs2/inode.h +1 -5

  	/* protected by recovery_lock. */
  	struct inode			*ip_next_orphan;
-
-	u32				ip_dir_start_lookup;
-
  	struct ocfs2_caching_info	ip_metadata_cache;
-
  	struct ocfs2_extent_map		ip_extent_map;
-
  	struct inode			vfs_inode;
  	struct jbd2_inode		ip_jinode;
+	u32				ip_dir_start_lookup;

reinstate the empty line after ip_next_orphan so that it does not give the impression that the other fields are also protected by recovery_lock. Also add empty lines around ip_dir_start_lookup.

Quoted 10 lines

fs/ocfs2/inode.h +0 -0

  	/* Only valid if the inode is the dir. */
  	u32				ip_last_used_slot;

fs/ocfs2/journal.h +0 -0

index b5baaa8..ed05ac3 100644
@@ -67,11 +67,11 @@ struct ocfs2_journal {
  	struct buffer_head        *j_bh;      /* Journal disk inode block */
  	atomic_t                  j_num_trans; /* Number of transactions

fs/ocfs2/journal.h struct ocfs2_journal { · line 77

Quoted 10 lines

fs/ocfs2/journal.h +1 -1

  					        * currently in the system. */
+	spinlock_t                j_lock;
  	unsigned long             j_trans_id;
  	struct rw_semaphore       j_trans_barrier;
  	wait_queue_head_t         j_checkpointed;
 
-	spinlock_t                j_lock;
  	struct list_head          j_la_cleanups;
  	struct work_struct        j_recovery_work;
  };

add a comment before j_la_cleanups saying "both protected by j_lock".

Quoted 25 lines

fs/ocfs2/ocfs2.h +1 -2

index c67003b..34b9c79 100644
@@ -151,17 +151,16 @@ struct ocfs2_lock_res {
  	void                    *l_priv;
  	struct ocfs2_lock_res_ops *l_ops;
  	spinlock_t               l_lock;
+	enum ocfs2_lock_type     l_type;
 
  	struct list_head         l_blocked_list;
  	struct list_head         l_mask_waiters;
 
-	enum ocfs2_lock_type     l_type;
  	unsigned long		 l_flags;
  	char                     l_name[OCFS2_LOCK_ID_MAX_LEN];
  	int                      l_level;
  	unsigned int             l_ro_holders;
  	unsigned int             l_ex_holders;
-	struct ocfs2_dlm_lksb    l_lksb;
 
  	/* used from AST/BAST funcs. */
  	enum ocfs2_ast_action    l_action;

fs/ocfs2/ocfs2.h struct ocfs2_lock_res { · line 175

Quoted 7 lines

fs/ocfs2/ocfs2.h +1 -0

@@ -170,6 +169,7 @@ struct ocfs2_lock_res {
  	int                      l_blocking;
  	unsigned int             l_pending_gen;
 
+	struct ocfs2_dlm_lksb    l_lksb;
  	wait_queue_head_t        l_event;
 
  	struct list_head         l_debug_list;

add empty lines around both l_lksb and l_type

Quoted 12 lines

fs/ocfs2/refcounttree.h +1 -0

index 9983ba1..67a89e4 100644
@@ -21,14 +21,14 @@ struct ocfs2_refcount_tree {
  	struct rb_node rf_node;
  	u64 rf_blkno;
  	u32 rf_generation;
+	struct kref rf_getcnt;
  	struct rw_semaphore rf_sem;
  	struct ocfs2_lock_res rf_lockres;

fs/ocfs2/refcounttree.h struct ocfs2_refcount_tree { · line 34

Quoted 10 lines

fs/ocfs2/refcounttree.h +1 -2

-	struct kref rf_getcnt;
  	int rf_removed;
+	spinlock_t rf_lock;
 
  	/* the following 4 fields are used by caching_info. */
  	struct ocfs2_caching_info rf_ci;
-	spinlock_t rf_lock;
  	struct mutex rf_io_mutex;
  	struct super_block *rf_sb;
  };

Move rf_lock below the comment.

Joel Becker

2010-06-10 00:45 UTC #2 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

In reply to Goldwyn Rodrigues

Quoted 11 lines · Goldwyn Rodrigues

This is the re-arrangement of the data elements of ocfs2 data structures to reduce memory consumption as shown by pahole on an x86_64 box. I have tried to keep the context as close as possible, though I was pretty agressive to get the numbers down.
Statistics in bytes: (before - after = reduction)
ocfs2_write_ctxt: 2144 - 2136 = 8
ocfs2_inode_info: 1960 - 1896 = 64
ocfs2_journal: 168 - 160 = 8
ocfs2_lock_res: 336 - 320 = 16
ocfs2_refcount_tree: 512 - 488 = 24

You should know that these won't actually affect ocfs2's memory
usage yet. All of our structures come from slabs, so they matter in multiples as they fit into slabs. What do I mean? When ocfs2_inode_info was 1960 bytes, you could fit two of them into a 4K page. Now that you've made it 1896 bytes, you can still only fit two of them into a 4K page. So you're still using the same number of pages.
However, every step we take to reducing the sizes gets us closer
to actual memory improvements. As an example, your change to ocfs2_lock_res reduces ocfs2_dentry_lock from 356 to 340 bytes on 32-bit. If we had a slab for dentry locks, that would go from 11 locks per slab to 12. Currently, though, we get them from kmalloc(). Because kmalloc() allocates in power-of-two chunks, we're using 512 byte allocations for all of our dentry locks. So a next step is to get dentry locks out to their own slab. Move the dl_count field to the end of the structure and you can pack 12 of them on 64-bit too. On top of your changes here, you would get a 50% usage improvement over the kmalloc() version (8 per kmalloc page to 12 per slab page).

fs/ocfs2/ocfs2.h struct ocfs2_lock_res { · line 154

Quoted 9 lines

fs/ocfs2/ocfs2.h +1 -0

index c67003b..34b9c79 100644
@@ -151,17 +151,16 @@ struct ocfs2_lock_res {
 	void                    *l_priv;
 	struct ocfs2_lock_res_ops *l_ops;
 	spinlock_t               l_lock;
+	enum ocfs2_lock_type     l_type;

I think you should change l_type, l_action, l_requested,
l_blocking, and l_level to unsigned char. While the enums that set them should be not modified, they do not have more than 256 values. All the functions around them can use the enum type in their arguments. Just the ocfs2_lock_res itself stores them in unsigned char.
This would potentially save us 15 bytes per ocfs2_lock_res,
45 per inode. More realistic is probably 12 per lock_res and 36 per inode, but still!
Here's the thing - we have more inodes and dentries than
anything else in memory, at least as far as the filesystem is concerned. Those are big wins.

Joel

Signature · 10 lines

"I'm drifting and drifting
 Just like a ship out on the sea.
 Cause I ain't got nobody, baby,
 In this world to care for me."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Joel Becker

2010-06-10 00:51 UTC #3 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

Quoted 11 lines · Joel Becker

fs/ocfs2/ocfs2.h struct ocfs2_lock_res { · line 154

Quoted 9 lines · Goldwyn Rodrigues

fs/ocfs2/ocfs2.h +1 -0

index c67003b..34b9c79 100644
@@ -151,17 +151,16 @@ struct ocfs2_lock_res {
 	void                    *l_priv;
 	struct ocfs2_lock_res_ops *l_ops;
 	spinlock_t               l_lock;
+	enum ocfs2_lock_type     l_type;

Quoted 8 lines

I think you should change l_type, l_action, l_requested,
l_blocking, and l_level to unsigned char. While the enums that set them should be not modified, they do not have more than 256 values. All the functions around them can use the enum type in their arguments. Just the ocfs2_lock_res itself stores them in unsigned char.
This would potentially save us 15 bytes per ocfs2_lock_res,
45 per inode. More realistic is probably 12 per lock_res and 36 per inode, but still!

If you do this, please comment that they are set from enums but
are unsigned char to save space because the value never overflows.

Joel

Signature · 10 lines

"Conservative, n.  A statesman who is enamoured of existing evils,
 as distinguished from the Liberal, who wishes to replace them
 with others."
	- Ambrose Bierce, The Devil's Dictionary

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Joel Becker

2010-06-10 00:54 UTC #4 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

In reply to Joel Becker

Quoted 9 lines · Joel Becker

fs/ocfs2/ocfs2.h struct ocfs2_lock_res { · line 154

Quoted 9 lines

fs/ocfs2/ocfs2.h +1 -0

index c67003b..34b9c79 100644
@@ -151,17 +151,16 @@ struct ocfs2_lock_res {
 	void                    *l_priv;
 	struct ocfs2_lock_res_ops *l_ops;
 	spinlock_t               l_lock;
+	enum ocfs2_lock_type     l_type;

On the subject of extreme structure diets... ocfs2_lock_res
uses ints for l_ro_holders and l_ex_holders. Do we ever have more than 64K holders? Can we squash those into u16s? I note that our code doesn't handle overflow at all, but 2^32 holders is significantly larger.
Goldwyn, did you test this with CONFIG_OCFS2_FS_STATS and
CONFIG_DEBUG_LOCK_ENABLED turned on or off? I'd be interested in that data too.

Joel

Signature · 10 lines

"But all my words come back to me
 In shades of mediocrity.
 Like emptiness in harmony
 I need someone to comfort me."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Goldwyn Rodrigues

author

2010-06-10 04:07 UTC #5 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

Hi Joel,

Quoted 6 lines · Joel Becker

? ? ? ?On the subject of extreme structure diets... ?ocfs2_lock_res uses ints for l_ro_holders and l_ex_holders. ?Do we ever have more than 64K holders? ?Can we squash those into u16s? ?I note that our code doesn't handle overflow at all, but 2^32 holders is significantly larger.

Good idea.

Quoted 4 lines

? ? ? ?Goldwyn, did you test this with CONFIG_OCFS2_FS_STATS and CONFIG_DEBUG_LOCK_ENABLED turned on or off? ?I'd be interested in that data too.

I had CONFIG_OCFS2_FS_STATS enabled and CONFIG_DEBUG_LOCK_ALLOC disabled.

Signature · 1 line

Goldwyn

Joel Becker

2010-06-10 00:56 UTC #6 lore raw

PATCH 1/1 Reoganize data elements to reduce memory footprint

In reply to Joel Becker

Quoted 2 lines · Joel Becker

I think you should change l_type, l_action, l_requested,
l_blocking, and l_level to unsigned char. While the enums that set them

I missed l_unlock_action. That one too.

Quoted 6 lines

should be not modified, they do not have more than 256 values. All the functions around them can use the enum type in their arguments. Just the ocfs2_lock_res itself stores them in unsigned char.
This would potentially save us 15 bytes per ocfs2_lock_res,
45 per inode. More realistic is probably 12 per lock_res and 36 per inode, but still!

So that's 18 bytes per ocfs2_lock_res, 54 per inode. Of course,
alignment probably costs us 2 per lock_res, so call it 48 per inode of real savings. Nice!

Joel

[Thanks to Tao for noticing I missed l_unlock_action]

Signature · 8 lines

"Here's a nickle -- get yourself a better X server."
	- Keith Packard

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127