Skip to contentSkip to conversation

btrfs: compress: put variables defined per compress type in struct to make cache friendly

PATCH Superseded Byongho Lee 2 participants 3 messages Oct 13, 2015 – Oct 14, 2015 Btrfs file system

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 btrfs: compress: put variables defined per compress type in struct to make cache friendly

Below variables are defined per compress type.
- struct list_head comp_idle_workspace[BTRFS_COMPRESS_TYPES]
- spinlock_t comp_workspace_lock[BTRFS_COMPRESS_TYPES]
- int comp_num_workspace[BTRFS_COMPRESS_TYPES]
- atomic_t comp_alloc_workspace[BTRFS_COMPRESS_TYPES]
- wait_queue_head_t comp_workspace_wait[BTRFS_COMPRESS_TYPES]

BTW, while accessing one compress type of these variables, the next or before address is other compress types of it.
So this patch puts these variables in a struct to make cache friendly.

---
 fs/btrfs/compression.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)
fs/btrfs/compression.c +24 -22
index ce62324c78e7..85a80931ae3f 100644
@@ -744,11 +744,13 @@ out:
 	return ret;
 }
 
-static struct list_head comp_idle_workspace[BTRFS_COMPRESS_TYPES];
-static spinlock_t comp_workspace_lock[BTRFS_COMPRESS_TYPES];
-static int comp_num_workspace[BTRFS_COMPRESS_TYPES];
-static atomic_t comp_alloc_workspace[BTRFS_COMPRESS_TYPES];
-static wait_queue_head_t comp_workspace_wait[BTRFS_COMPRESS_TYPES];
+static struct {
+	struct list_head idle_workspace;
+	spinlock_t workspace_lock;
+	int num_workspace;
+	atomic_t alloc_workspace;
+	wait_queue_head_t workspace_wait;
+} comp[BTRFS_COMPRESS_TYPES];
 
 static const struct btrfs_compress_op * const btrfs_compress_op[] = {
 	&btrfs_zlib_compress,
@@ -760,10 +762,10 @@ void __init btrfs_init_compress(void)
 	int i;
 
 	for (i = 0; i < BTRFS_COMPRESS_TYPES; i++) {
-		INIT_LIST_HEAD(&comp_idle_workspace[i]);
-		spin_lock_init(&comp_workspace_lock[i]);
-		atomic_set(&comp_alloc_workspace[i], 0);
-		init_waitqueue_head(&comp_workspace_wait[i]);
+		INIT_LIST_HEAD(&comp[i].idle_workspace);
+		spin_lock_init(&comp[i].workspace_lock);
+		atomic_set(&comp[i].alloc_workspace, 0);
+		init_waitqueue_head(&comp[i].workspace_wait);
 	}
 }
 
@@ -777,11 +779,11 @@ static struct list_head *find_workspace(int type)
 	int cpus = num_online_cpus();
 	int idx = type - 1;
 
-	struct list_head *idle_workspace	= &comp_idle_workspace[idx];
-	spinlock_t *workspace_lock		= &comp_workspace_lock[idx];
-	atomic_t *alloc_workspace		= &comp_alloc_workspace[idx];
-	wait_queue_head_t *workspace_wait	= &comp_workspace_wait[idx];
-	int *num_workspace			= &comp_num_workspace[idx];
+	struct list_head *idle_workspace	= &comp[idx].idle_workspace;
+	spinlock_t *workspace_lock		= &comp[idx].workspace_lock;
+	atomic_t *alloc_workspace		= &comp[idx].alloc_workspace;
+	wait_queue_head_t *workspace_wait	= &comp[idx].workspace_wait;
+	int *num_workspace			= &comp[idx].num_workspace;
 again:
 	spin_lock(workspace_lock);
 	if (!list_empty(idle_workspace)) {
@@ -820,11 +822,11 @@ again:
 static void free_workspace(int type, struct list_head *workspace)
 {
 	int idx = type - 1;
-	struct list_head *idle_workspace	= &comp_idle_workspace[idx];
-	spinlock_t *workspace_lock		= &comp_workspace_lock[idx];
-	atomic_t *alloc_workspace		= &comp_alloc_workspace[idx];
-	wait_queue_head_t *workspace_wait	= &comp_workspace_wait[idx];
-	int *num_workspace			= &comp_num_workspace[idx];
+	struct list_head *idle_workspace	= &comp[idx].idle_workspace;
+	spinlock_t *workspace_lock		= &comp[idx].workspace_lock;
+	atomic_t *alloc_workspace		= &comp[idx].alloc_workspace;
+	wait_queue_head_t *workspace_wait	= &comp[idx].workspace_wait;
+	int *num_workspace			= &comp[idx].num_workspace;
 
 	spin_lock(workspace_lock);
 	if (*num_workspace < num_online_cpus()) {
@@ -852,11 +854,11 @@ static void free_workspaces(void)
 	int i;
 
 	for (i = 0; i < BTRFS_COMPRESS_TYPES; i++) {
-		while (!list_empty(&comp_idle_workspace[i])) {
-			workspace = comp_idle_workspace[i].next;
+		while (!list_empty(&comp[i].idle_workspace)) {
+			workspace = comp[i].idle_workspace.next;
 			list_del(workspace);
 			btrfs_compress_op[i]->free_workspace(workspace);
-			atomic_dec(&comp_alloc_workspace[i]);
+			atomic_dec(&comp[i].alloc_workspace);
 		}
 	}
 }
Signature · 1 line
2.6.1
#1 lore raw
Quoted 10 lines · Byongho Lee

Below variables are defined per compress type.
- struct list_head comp_idle_workspace[BTRFS_COMPRESS_TYPES]
- spinlock_t comp_workspace_lock[BTRFS_COMPRESS_TYPES]
- int comp_num_workspace[BTRFS_COMPRESS_TYPES]
- atomic_t comp_alloc_workspace[BTRFS_COMPRESS_TYPES]
- wait_queue_head_t comp_workspace_wait[BTRFS_COMPRESS_TYPES]

BTW, while accessing one compress type of these variables, the next or before address is other compress types of it.
So this patch puts these variables in a struct to make cache friendly.

Nice.

fs/btrfs/compression.c out: · line 753

Quoted 7 lines
fs/btrfs/compression.c +7 -0
+static struct {
+	struct list_head idle_workspace;
+	spinlock_t workspace_lock;
+	int num_workspace;
+	atomic_t alloc_workspace;
+	wait_queue_head_t workspace_wait;
+} comp[BTRFS_COMPRESS_TYPES];

The name became too generic, please rename it to btrfs_comp_ws. btrfs_comp_workspaces would be too long. I won't mind trimming the members to 'ws' instead of 'workspace' so this does not result in too wild code formatting. The use of the workspaces is localized only to the compression code so it will not be confusing.

#2 lore raw
Quoted 14 lines · David Sterba

fs/btrfs/compression.c out: · line 753

Quoted 8 lines
fs/btrfs/compression.c +7 -0
+static struct {
+	struct list_head idle_workspace;
+	spinlock_t workspace_lock;
+	int num_workspace;
+	atomic_t alloc_workspace;
+	wait_queue_head_t workspace_wait;
+} comp[BTRFS_COMPRESS_TYPES];

The name became too generic, please rename it to btrfs_comp_ws. btrfs_comp_workspaces would be too long. I won't mind trimming the members to 'ws' instead of 'workspace' so this does not result in too wild code formatting. The use of the workspaces is localized only to the compression code so it will not be confusing.

Thanks for feedback.
I will prepare v2 patch applying your comment.