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,
On Wed, Dec 17, 2008 at 7:48 PM, Alasdair G Kergon <[email protected]> wrote:
> 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
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(-)
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 835cf95..018df35 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -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;
}
--
1.6.0.2.GIT
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(-)
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 018df35..a713c7f 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -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;
}
--
1.6.0.2.GIT
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(-)
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index a713c7f..ee120a9 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -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;
}
/*
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index c17fd33..8bcc7f5 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -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 {
--
1.6.0.2.GIT
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
--
[email protected]
On Sat, Jan 3, 2009 at 2:12 AM, Alasdair G Kergon <[email protected]> wrote:> 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 onemonth ago, I want to know you (dm-dev)'s attitude toward this type ofstruct reconstruction. Now I know it and can do more.
>> Alasdair> --> [email protected]
BTW, why you set Reply-to field to my email address? Maybe an accidentof your mail client?
-- Cheng Renquan (程任全), Shenzhen, ChinaGroucho Marx - "I was married by a judge. I should have asked for a jury."????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?