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

Commit Message

Byongho Lee Oct. 13, 2015, 4:13 p.m. UTC

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.

Signed-off-by: Byongho Lee <[e-mail]>
---
 fs/btrfs/compression.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

Comments

David Sterba Oct. 13, 2015, 4:28 p.m. UTC | #1

On Wed, Oct 14, 2015 at 01:13:26AM +0900, Byongho Lee wrote:
> 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.

> +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Byongho Lee Oct. 14, 2015, 1:17 a.m. UTC | #2

David Sterba writes:

>
>> +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ce62324c78e7..85a80931ae3f 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -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);
 		}
 	}
 }