dm-target: target_type improvements

PATCH In review [email protected] 2 participants 6 messages Dec 19, 2008 – Jan 3, 2009 Device mapper

lore

Share & tools

lore

[email protected]

author

2008-12-19 04:19 UTC #0 lore raw

cover dm-target: target_type improvements

From: Cheng Renquan <[email protected]>

The series of patches:
1. use module's refcount instead of self-maintained use field;
2. use pointer reference instead of making a copy of target_type;
3. totally remove tt_internal;

The 3rd patch may be controversial,

Quoted 8 lines · Alasdair G Kergon

Target registrations should be rare one-off events. The existing trade-off is
in favour of a cleaner interface (that does not expose private fields). The struct target_type passed to dm_register_target is always static read-only data
and perhaps that could be enforced and a pointer stored in tt_internal instead
of making a copy.

But I still think it's worth it:
1. current users of struct target_type hasn't been marked with const, just static;
2. other similar structures (file_system_type in filesystems, packet_type in net core, ) all have internally used list_head for manage purpose; this design can avoid memory frag from kmalloc/kfree.

--
Cheng Renquan, Shenzhen, China

[email protected]

author

2008-12-19 04:19 UTC #1 lore raw

PATCH 1/3 dm-target: use the module's refcount instead

From: Cheng Renquan <[email protected]>

The tt_internal's use field is superfluous, the module's refcount has done the work properly.

Signed-off-by Cheng Renquan <[email protected]>

---
 drivers/md/dm-target.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

drivers/md/dm-target.c +4 ~~-12~~

index 835cf95..018df35 100644
@@ -18,7 +18,6 @@ struct tt_internal {
 	struct target_type tt;
 
 	struct list_head list;
-	long use;
 };
 
 static LIST_HEAD(_targets);

@@ -44,12 +43,8 @@ static struct tt_internal *get_target_type(const char *name)
 	down_read(&_lock);
 
 	ti = __find_target_type(name);
-	if (ti) {
-		if ((ti->use == 0) && !try_module_get(ti->tt.module))
-			ti = NULL;
-		else
-			ti->use++;
-	}
+	if (ti && !try_module_get(ti->tt.module))
+		ti = NULL;
 
 	up_read(&_lock);
 	return ti;

@@ -77,10 +72,7 @@ void dm_put_target_type(struct target_type *t)
 	struct tt_internal *ti = (struct tt_internal *) t;
 
 	down_read(&_lock);
-	if (--ti->use == 0)
-		module_put(ti->tt.module);
-
-	BUG_ON(ti->use < 0);
+	module_put(ti->tt.module);
 	up_read(&_lock);
 
 	return;

@@ -140,7 +132,7 @@ int dm_unregister_target(struct target_type *t)
 		return -EINVAL;
 	}
 
-	if (ti->use) {
+	if (ti->tt.module && module_refcount(ti->tt.module)) {
 		up_write(&_lock);
 		return -ETXTBSY;
 	}

Signature · 1 line

1.6.0.2.GIT

[email protected]

author

2008-12-19 04:19 UTC #2 lore raw

PATCH 2/3 dm-target: use pointer reference instead of making a copy

From: Cheng Renquan <[email protected]>

The tt_internal always use target_type read-only, so there's no need to making a copy, just a pointer reference is good, this could save
(sizeof(target_type) - sizeof(pointer)) bytes memory per target_type.

  -		ti->tt = *t;
  +		ti->tt = t;

Signed-off-by Cheng Renquan <[email protected]>

---
 drivers/md/dm-target.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

drivers/md/dm-target.c +8 -8

index 018df35..a713c7f 100644
@@ -15,7 +15,7 @@
 #define DM_MSG_PREFIX "target"
 
 struct tt_internal {
-	struct target_type tt;
+	struct target_type *tt;
 
 	struct list_head list;
 };

@@ -30,7 +30,7 @@ static inline struct tt_internal *__find_target_type(const char *name)
 	struct tt_internal *ti;
 
 	list_for_each_entry (ti, &_targets, list)
-		if (!strcmp(name, ti->tt.name))
+		if (!strcmp(name, ti->tt->name))
 			return ti;
 
 	return NULL;

@@ -43,7 +43,7 @@ static struct tt_internal *get_target_type(const char *name)
 	down_read(&_lock);
 
 	ti = __find_target_type(name);
-	if (ti && !try_module_get(ti->tt.module))
+	if (ti && !try_module_get(ti->tt->module))
 		ti = NULL;
 
 	up_read(&_lock);

@@ -64,7 +64,7 @@ struct target_type *dm_get_target_type(const char *name)
 		ti = get_target_type(name);
 	}
 
-	return ti ? &ti->tt : NULL;
+	return ti ? ti->tt : NULL;
 }
 
 void dm_put_target_type(struct target_type *t)

@@ -72,7 +72,7 @@ void dm_put_target_type(struct target_type *t)
 	struct tt_internal *ti = (struct tt_internal *) t;
 
 	down_read(&_lock);
-	module_put(ti->tt.module);
+	module_put(ti->tt->module);
 	up_read(&_lock);
 
 	return;

@@ -83,7 +83,7 @@ static struct tt_internal *alloc_target(struct target_type *t)
 	struct tt_internal *ti = kzalloc(sizeof(*ti), GFP_KERNEL);
 
 	if (ti)
-		ti->tt = *t;
+		ti->tt = t;
 
 	return ti;
 }

@@ -96,7 +96,7 @@ int dm_target_iterate(void (*iter_func)(struct target_type *tt,
 
 	down_read(&_lock);
 	list_for_each_entry (ti, &_targets, list)
-		iter_func(&ti->tt, param);
+		iter_func(ti->tt, param);
 	up_read(&_lock);
 
 	return 0;

@@ -132,7 +132,7 @@ int dm_unregister_target(struct target_type *t)
 		return -EINVAL;
 	}
 
-	if (ti->tt.module && module_refcount(ti->tt.module)) {
+	if (ti->tt->module && module_refcount(ti->tt->module)) {
 		up_write(&_lock);
 		return -ETXTBSY;
 	}

Signature · 1 line

1.6.0.2.GIT

[email protected]

author

2008-12-19 04:19 UTC #3 lore raw

PATCH 3/3 dm-target: embed internally used list_head into target_type

From: Cheng Renquan <[email protected]>

The tt_internal is really just a list_head to manage registered target_type in a double linked list,

Here embed the list_head into target_type directly,
1. to avoid kmalloc/kfree;
2. then tt_internal is really unneeded;

Signed-off-by Cheng Renquan <[email protected]>

---
 drivers/md/dm-target.c        |   95 +++++++++++++++--------------------------
 include/linux/device-mapper.h |    3 +
 2 files changed, 37 insertions(+), 61 deletions(-)

drivers/md/dm-target.c +34 ~~-61~~

index a713c7f..ee120a9 100644
@@ -14,40 +14,34 @@
 
 #define DM_MSG_PREFIX "target"
 
-struct tt_internal {
-	struct target_type *tt;
-
-	struct list_head list;
-};
-
 static LIST_HEAD(_targets);
 static DECLARE_RWSEM(_lock);
 
 #define DM_MOD_NAME_SIZE 32
 
-static inline struct tt_internal *__find_target_type(const char *name)
+static inline struct target_type *__find_target_type(const char *name)
 {
-	struct tt_internal *ti;
+	struct target_type *tt;
 
-	list_for_each_entry (ti, &_targets, list)
-		if (!strcmp(name, ti->tt->name))
-			return ti;
+	list_for_each_entry (tt, &_targets, list)
+		if (!strcmp(name, tt->name))
+			return tt;
 
 	return NULL;
 }
 
-static struct tt_internal *get_target_type(const char *name)
+static struct target_type *get_target_type(const char *name)
 {
-	struct tt_internal *ti;
+	struct target_type *tt;
 
 	down_read(&_lock);
 
-	ti = __find_target_type(name);
-	if (ti && !try_module_get(ti->tt->module))
-		ti = NULL;
+	tt = __find_target_type(name);
+	if (tt && !try_module_get(tt->module))
+		tt = NULL;
 
 	up_read(&_lock);
-	return ti;
+	return tt;
 }
 
 static void load_module(const char *name)

@@ -57,91 +51,70 @@ static void load_module(const char *name)
 
 struct target_type *dm_get_target_type(const char *name)
 {
-	struct tt_internal *ti = get_target_type(name);
+	struct target_type *tt = get_target_type(name);
 
-	if (!ti) {
+	if (!tt) {
 		load_module(name);
-		ti = get_target_type(name);
+		tt = get_target_type(name);
 	}
 
-	return ti ? ti->tt : NULL;
+	return tt;
 }
 
-void dm_put_target_type(struct target_type *t)
+void dm_put_target_type(struct target_type *tt)
 {
-	struct tt_internal *ti = (struct tt_internal *) t;
-
 	down_read(&_lock);
-	module_put(ti->tt->module);
+	module_put(tt->module);
 	up_read(&_lock);
-
-	return;
 }
 
-static struct tt_internal *alloc_target(struct target_type *t)
-{
-	struct tt_internal *ti = kzalloc(sizeof(*ti), GFP_KERNEL);
-
-	if (ti)
-		ti->tt = t;
-
-	return ti;
-}
-
-
 int dm_target_iterate(void (*iter_func)(struct target_type *tt,
 					void *param), void *param)
 {
-	struct tt_internal *ti;
+	struct target_type *tt;
 
 	down_read(&_lock);
-	list_for_each_entry (ti, &_targets, list)
-		iter_func(ti->tt, param);
+	list_for_each_entry (tt, &_targets, list)
+		iter_func(tt, param);
 	up_read(&_lock);
 
 	return 0;
 }
 
-int dm_register_target(struct target_type *t)
+int dm_register_target(struct target_type *tt)
 {
 	int rv = 0;
-	struct tt_internal *ti = alloc_target(t);
-
-	if (!ti)
-		return -ENOMEM;
 
 	down_write(&_lock);
-	if (__find_target_type(t->name))
+	if (__find_target_type(tt->name))
 		rv = -EEXIST;
 	else
-		list_add(&ti->list, &_targets);
+		list_add(&tt->list, &_targets);
 
 	up_write(&_lock);
-	if (rv)
-		kfree(ti);
 	return rv;
 }
 
-int dm_unregister_target(struct target_type *t)
+int dm_unregister_target(struct target_type *tt)
 {
-	struct tt_internal *ti;
+	int rv = 0;
 
 	down_write(&_lock);
-	if (!(ti = __find_target_type(t->name))) {
-		up_write(&_lock);
-		return -EINVAL;
+	if (!__find_target_type(tt->name)) {
+		rv = -EINVAL;
+		goto out;
 	}
 
-	if (ti->tt->module && module_refcount(ti->tt->module)) {
-		up_write(&_lock);
-		return -ETXTBSY;
+	if (tt->module && module_refcount(tt->module)) {
+		rv = -ETXTBSY;
+		goto out;
 	}
 
-	list_del(&ti->list);
-	kfree(ti);
+	list_del(&tt->list);
 
+ out:
 	up_write(&_lock);
-	return 0;
+	return rv;
 }
 
 /*

include/linux/device-mapper.h +3 -0

index c17fd33..8bcc7f5 100644
@@ -117,6 +117,9 @@ struct target_type {
 	dm_message_fn message;
 	dm_ioctl_fn ioctl;
 	dm_merge_fn merge;
+
+	/* for internal use only */
+	struct list_head list;
 };
 
 struct io_restrictions {

Signature · 1 line

1.6.0.2.GIT

Alasdair G Kergon

maintainer

2009-01-02 18:12 UTC #4 lore raw

In reply to [email protected]

Quoted 4 lines · On Fri, Dec 19, 2008 at 12:19:42PM +0800, [email protected] wrote:

From: Cheng Renquan <[email protected]>
3. totally remove tt_internal;
The 3rd patch may be controversial,
But I still think it's worth it:

Well now that it's down to just one field for linking items I'm inclined to agree that it's OK.

Do you (or anyone else) have any interest in creating and testing similar sets of patches for the other parts of device-mapper that use the same construction?

Alasdair

Signature · 1 line

[email protected]

Cheng Renquan (程任全)

author

2009-01-03 06:42 UTC #5 lore raw

Quoted 11 lines · Alasdair G Kergon

Quoted 4 lines · On Fri, Dec 19, 2008 at 12:19:42PM +0800, [email protected] wrote:
From: Cheng Renquan <[email protected]>
3. totally remove tt_internal;
The 3rd patch may be controversial,
But I still think it's worth it:
Well now that it's down to just one field for linking items I'm inclined to agree that it's OK.
Do you (or anyone else) have any interest in creating and testing similar sets of patches for the other parts of device-mapper that use the same construction?

I just came across the device-mapper code and made the patches one month ago, I want to know you (dm-dev)'s attitude toward this type of struct reconstruction. Now I know it and can do more.

Quoted 4 lines

Alasdair
--
[email protected]

BTW, why you set Reply-to field to my email address? Maybe an accident of your mail client?

Signature · 2 lines

Cheng Renquan (程任全), Shenzhen, China
Groucho Marx  - "I was married by a judge. I should have asked for a jury."