* [PATCH 0/3] dm-target: target_type improvements
@ 2008-12-19 4:19 crquan
2008-12-19 4:19 ` [PATCH 1/3] dm-target: use the module's refcount instead crquan
2009-01-02 18:12 ` [dm-devel] [PATCH 0/3] dm-target: target_type improvements Alasdair G Kergon
0 siblings, 2 replies; 6+ messages in thread
From: crquan @ 2008-12-19 4:19 UTC (permalink / raw)
To: dm-devel; +Cc: Alasdair G Kergon, linux-kernel
From: Cheng Renquan <crquan@gmail.com>
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 <agk@redhat.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/3] dm-target: use the module's refcount instead 2008-12-19 4:19 [PATCH 0/3] dm-target: target_type improvements crquan @ 2008-12-19 4:19 ` crquan 2008-12-19 4:19 ` [PATCH 2/3] dm-target: use pointer reference instead of making a copy crquan 2009-01-02 18:12 ` [dm-devel] [PATCH 0/3] dm-target: target_type improvements Alasdair G Kergon 1 sibling, 1 reply; 6+ messages in thread From: crquan @ 2008-12-19 4:19 UTC (permalink / raw) To: dm-devel; +Cc: Alasdair G Kergon, linux-kernel From: Cheng Renquan <crquan@gmail.com> The tt_internal's use field is superfluous, the module's refcount has done the work properly. Signed-off-by: Cheng Renquan <crquan@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] dm-target: use pointer reference instead of making a copy 2008-12-19 4:19 ` [PATCH 1/3] dm-target: use the module's refcount instead crquan @ 2008-12-19 4:19 ` crquan 2008-12-19 4:19 ` [PATCH 3/3] dm-target: embed internally used list_head into target_type crquan 0 siblings, 1 reply; 6+ messages in thread From: crquan @ 2008-12-19 4:19 UTC (permalink / raw) To: dm-devel; +Cc: Alasdair G Kergon, linux-kernel From: Cheng Renquan <crquan@gmail.com> 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 <crquan@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] dm-target: embed internally used list_head into target_type 2008-12-19 4:19 ` [PATCH 2/3] dm-target: use pointer reference instead of making a copy crquan @ 2008-12-19 4:19 ` crquan 0 siblings, 0 replies; 6+ messages in thread From: crquan @ 2008-12-19 4:19 UTC (permalink / raw) To: dm-devel; +Cc: Alasdair G Kergon, linux-kernel From: Cheng Renquan <crquan@gmail.com> 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 <crquan@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH 0/3] dm-target: target_type improvements 2008-12-19 4:19 [PATCH 0/3] dm-target: target_type improvements crquan 2008-12-19 4:19 ` [PATCH 1/3] dm-target: use the module's refcount instead crquan @ 2009-01-02 18:12 ` Alasdair G Kergon 2009-01-03 6:42 ` Cheng Renquan (程任全) 1 sibling, 1 reply; 6+ messages in thread From: Alasdair G Kergon @ 2009-01-02 18:12 UTC (permalink / raw) To: Cheng Renquan; +Cc: linux-kernel, device-mapper development On Fri, Dec 19, 2008 at 12:19:42PM +0800, crquan@gmail.com wrote: > From: Cheng Renquan <crquan@gmail.com> > 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 -- agk@redhat.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH 0/3] dm-target: target_type improvements 2009-01-02 18:12 ` [dm-devel] [PATCH 0/3] dm-target: target_type improvements Alasdair G Kergon @ 2009-01-03 6:42 ` Cheng Renquan (程任全) 0 siblings, 0 replies; 6+ messages in thread From: Cheng Renquan (程任全) @ 2009-01-03 6:42 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: device-mapper development, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1141 bytes --] On Sat, Jan 3, 2009 at 2:12 AM, Alasdair G Kergon <agk@redhat.com> wrote:> On Fri, Dec 19, 2008 at 12:19:42PM +0800, crquan@gmail.com wrote:>> From: Cheng Renquan <crquan@gmail.com>>> 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> --> agk@redhat.com 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Ç+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-01-03 6:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-12-19 4:19 [PATCH 0/3] dm-target: target_type improvements crquan 2008-12-19 4:19 ` [PATCH 1/3] dm-target: use the module's refcount instead crquan 2008-12-19 4:19 ` [PATCH 2/3] dm-target: use pointer reference instead of making a copy crquan 2008-12-19 4:19 ` [PATCH 3/3] dm-target: embed internally used list_head into target_type crquan 2009-01-02 18:12 ` [dm-devel] [PATCH 0/3] dm-target: target_type improvements Alasdair G Kergon 2009-01-03 6:42 ` Cheng Renquan (程任全)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox