| Message ID | 1523960140-86687-1-git-send-email-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 17, 2018 at 12:15 PM, Chengguang Xu <cgxu519@gmx.com> wrote: > Do memory allocation first, so that avoid unnecessary > initialization of newly allocated message in error case. > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > --- > net/ceph/messenger.c | 42 ++++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 22 deletions(-) > > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c > index fcb40c1..5a8c5cd 100644 > --- a/net/ceph/messenger.c > +++ b/net/ceph/messenger.c > @@ -3331,6 +3331,16 @@ void ceph_msg_data_add_bvecs(struct ceph_msg *msg, > EXPORT_SYMBOL(ceph_msg_data_add_bvecs); > > /* > + * Free a generically kmalloc'd message. > + */ > +static void ceph_msg_free(struct ceph_msg *m) > +{ > + dout("%s %p\n", __func__, m); > + kvfree(m->front.iov_base); > + kmem_cache_free(ceph_msg_cache, m); > +} > + > +/* > * construct a new message with given type, size > * the new msg has a ref count of 1. > */ > @@ -3343,14 +3353,6 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, > if (m == NULL) > goto out; > > - m->hdr.type = cpu_to_le16(type); > - m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); > - m->hdr.front_len = cpu_to_le32(front_len); > - > - INIT_LIST_HEAD(&m->list_head); > - kref_init(&m->kref); > - INIT_LIST_HEAD(&m->data); > - > /* front */ > if (front_len) { > m->front.iov_base = ceph_kvmalloc(front_len, flags); > @@ -3359,16 +3361,23 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, > front_len); > goto out2; > } > - } else { > - m->front.iov_base = NULL; > } > + > + m->hdr.type = cpu_to_le16(type); > + m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); > + m->hdr.front_len = cpu_to_le32(front_len); > + > + INIT_LIST_HEAD(&m->list_head); > + kref_init(&m->kref); > + INIT_LIST_HEAD(&m->data); > + > m->front_alloc_len = m->front.iov_len = front_len; > > dout("ceph_msg_new %p front %d\n", m, front_len); > return m; > > out2: > - ceph_msg_put(m); > + ceph_msg_free(m); > out: > if (!can_fail) { > pr_err("msg_new can't create type %d front %d\n", type, > @@ -3467,17 +3476,6 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) > return ret; > } > > - > -/* > - * Free a generically kmalloc'd message. > - */ > -static void ceph_msg_free(struct ceph_msg *m) > -{ > - dout("%s %p\n", __func__, m); > - kvfree(m->front.iov_base); > - kmem_cache_free(ceph_msg_cache, m); > -} > - > static void ceph_msg_release(struct kref *kref) > { > struct ceph_msg *m = container_of(kref, struct ceph_msg, kref); Hi Chengguang, I don't think this buys anything. The difference is literally a few integer/pointer assignments in the error case, which means a failed OSD or MDS request. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 在 2018年4月17日，下午8:46，Ilya Dryomov <idryomov@gmail.com> 写道： > > On Tue, Apr 17, 2018 at 12:15 PM, Chengguang Xu <cgxu519@gmx.com> wrote: >> Do memory allocation first, so that avoid unnecessary >> initialization of newly allocated message in error case. >> >> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >> --- >> net/ceph/messenger.c | 42 ++++++++++++++++++++---------------------- >> 1 file changed, 20 insertions(+), 22 deletions(-) >> >> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c >> index fcb40c1..5a8c5cd 100644 >> --- a/net/ceph/messenger.c >> +++ b/net/ceph/messenger.c >> @@ -3331,6 +3331,16 @@ void ceph_msg_data_add_bvecs(struct ceph_msg *msg, >> EXPORT_SYMBOL(ceph_msg_data_add_bvecs); >> >> /* >> + * Free a generically kmalloc'd message. >> + */ >> +static void ceph_msg_free(struct ceph_msg *m) >> +{ >> + dout("%s %p\n", __func__, m); >> + kvfree(m->front.iov_base); >> + kmem_cache_free(ceph_msg_cache, m); >> +} >> + >> +/* >> * construct a new message with given type, size >> * the new msg has a ref count of 1. >> */ >> @@ -3343,14 +3353,6 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, >> if (m == NULL) >> goto out; >> >> - m->hdr.type = cpu_to_le16(type); >> - m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); >> - m->hdr.front_len = cpu_to_le32(front_len); >> - >> - INIT_LIST_HEAD(&m->list_head); >> - kref_init(&m->kref); >> - INIT_LIST_HEAD(&m->data); >> - >> /* front */ >> if (front_len) { >> m->front.iov_base = ceph_kvmalloc(front_len, flags); >> @@ -3359,16 +3361,23 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, >> front_len); >> goto out2; >> } >> - } else { >> - m->front.iov_base = NULL; >> } >> + >> + m->hdr.type = cpu_to_le16(type); >> + m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); >> + m->hdr.front_len = cpu_to_le32(front_len); >> + >> + INIT_LIST_HEAD(&m->list_head); >> + kref_init(&m->kref); >> + INIT_LIST_HEAD(&m->data); >> + >> m->front_alloc_len = m->front.iov_len = front_len; >> >> dout("ceph_msg_new %p front %d\n", m, front_len); >> return m; >> >> out2: >> - ceph_msg_put(m); >> + ceph_msg_free(m); >> out: >> if (!can_fail) { >> pr_err("msg_new can't create type %d front %d\n", type, >> @@ -3467,17 +3476,6 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) >> return ret; >> } >> >> - >> -/* >> - * Free a generically kmalloc'd message. >> - */ >> -static void ceph_msg_free(struct ceph_msg *m) >> -{ >> - dout("%s %p\n", __func__, m); >> - kvfree(m->front.iov_base); >> - kmem_cache_free(ceph_msg_cache, m); >> -} >> - >> static void ceph_msg_release(struct kref *kref) >> { >> struct ceph_msg *m = container_of(kref, struct ceph_msg, kref); > > Hi Chengguang, > > I don't think this buys anything. The difference is literally a few > integer/pointer assignments in the error case, which means a failed OSD > or MDS request. Maybe the commit log is not so well. IMO, ceph_msg_new() is low level function, so when failing allocating msg, it’s better to call ceph_msg_free() instead of ceph_msg_put() so that we can also avoid unnecessary check and operations in releasing msg. Thanks, Chengguang. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 17, 2018 at 3:13 PM, cgxu519@gmx.com <cgxu519@gmx.com> wrote: > >> 在 2018年4月17日，下午8:46，Ilya Dryomov <idryomov@gmail.com> 写道： >> >> On Tue, Apr 17, 2018 at 12:15 PM, Chengguang Xu <cgxu519@gmx.com> wrote: >>> Do memory allocation first, so that avoid unnecessary >>> initialization of newly allocated message in error case. >>> >>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >>> --- >>> net/ceph/messenger.c | 42 ++++++++++++++++++++---------------------- >>> 1 file changed, 20 insertions(+), 22 deletions(-) >>> >>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c >>> index fcb40c1..5a8c5cd 100644 >>> --- a/net/ceph/messenger.c >>> +++ b/net/ceph/messenger.c >>> @@ -3331,6 +3331,16 @@ void ceph_msg_data_add_bvecs(struct ceph_msg *msg, >>> EXPORT_SYMBOL(ceph_msg_data_add_bvecs); >>> >>> /* >>> + * Free a generically kmalloc'd message. >>> + */ >>> +static void ceph_msg_free(struct ceph_msg *m) >>> +{ >>> + dout("%s %p\n", __func__, m); >>> + kvfree(m->front.iov_base); >>> + kmem_cache_free(ceph_msg_cache, m); >>> +} >>> + >>> +/* >>> * construct a new message with given type, size >>> * the new msg has a ref count of 1. >>> */ >>> @@ -3343,14 +3353,6 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, >>> if (m == NULL) >>> goto out; >>> >>> - m->hdr.type = cpu_to_le16(type); >>> - m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); >>> - m->hdr.front_len = cpu_to_le32(front_len); >>> - >>> - INIT_LIST_HEAD(&m->list_head); >>> - kref_init(&m->kref); >>> - INIT_LIST_HEAD(&m->data); >>> - >>> /* front */ >>> if (front_len) { >>> m->front.iov_base = ceph_kvmalloc(front_len, flags); >>> @@ -3359,16 +3361,23 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, >>> front_len); >>> goto out2; >>> } >>> - } else { >>> - m->front.iov_base = NULL; >>> } >>> + >>> + m->hdr.type = cpu_to_le16(type); >>> + m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); >>> + m->hdr.front_len = cpu_to_le32(front_len); >>> + >>> + INIT_LIST_HEAD(&m->list_head); >>> + kref_init(&m->kref); >>> + INIT_LIST_HEAD(&m->data); >>> + >>> m->front_alloc_len = m->front.iov_len = front_len; >>> >>> dout("ceph_msg_new %p front %d\n", m, front_len); >>> return m; >>> >>> out2: >>> - ceph_msg_put(m); >>> + ceph_msg_free(m); >>> out: >>> if (!can_fail) { >>> pr_err("msg_new can't create type %d front %d\n", type, >>> @@ -3467,17 +3476,6 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) >>> return ret; >>> } >>> >>> - >>> -/* >>> - * Free a generically kmalloc'd message. >>> - */ >>> -static void ceph_msg_free(struct ceph_msg *m) >>> -{ >>> - dout("%s %p\n", __func__, m); >>> - kvfree(m->front.iov_base); >>> - kmem_cache_free(ceph_msg_cache, m); >>> -} >>> - >>> static void ceph_msg_release(struct kref *kref) >>> { >>> struct ceph_msg *m = container_of(kref, struct ceph_msg, kref); >> >> Hi Chengguang, >> >> I don't think this buys anything. The difference is literally a few >> integer/pointer assignments in the error case, which means a failed OSD >> or MDS request. > > > Maybe the commit log is not so well. IMO, ceph_msg_new() is low level function, > so when failing allocating msg, it’s better to call ceph_msg_free() instead of > ceph_msg_put() so that we can also avoid unnecessary check and operations in > releasing msg. This is an unlikely error case, it doesn't need to be optimized. In general, if the struct is kref-ed, it's better to always use "put" to stay consistent and avoid confusion. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index fcb40c1..5a8c5cd 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -3331,6 +3331,16 @@ void ceph_msg_data_add_bvecs(struct ceph_msg *msg, EXPORT_SYMBOL(ceph_msg_data_add_bvecs); /* + * Free a generically kmalloc'd message. + */ +static void ceph_msg_free(struct ceph_msg *m) +{ + dout("%s %p\n", __func__, m); + kvfree(m->front.iov_base); + kmem_cache_free(ceph_msg_cache, m); +} + +/* * construct a new message with given type, size * the new msg has a ref count of 1. */ @@ -3343,14 +3353,6 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, if (m == NULL) goto out; - m->hdr.type = cpu_to_le16(type); - m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); - m->hdr.front_len = cpu_to_le32(front_len); - - INIT_LIST_HEAD(&m->list_head); - kref_init(&m->kref); - INIT_LIST_HEAD(&m->data); - /* front */ if (front_len) { m->front.iov_base = ceph_kvmalloc(front_len, flags); @@ -3359,16 +3361,23 @@ struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags, front_len); goto out2; } - } else { - m->front.iov_base = NULL; } + + m->hdr.type = cpu_to_le16(type); + m->hdr.priority = cpu_to_le16(CEPH_MSG_PRIO_DEFAULT); + m->hdr.front_len = cpu_to_le32(front_len); + + INIT_LIST_HEAD(&m->list_head); + kref_init(&m->kref); + INIT_LIST_HEAD(&m->data); + m->front_alloc_len = m->front.iov_len = front_len; dout("ceph_msg_new %p front %d\n", m, front_len); return m; out2: - ceph_msg_put(m); + ceph_msg_free(m); out: if (!can_fail) { pr_err("msg_new can't create type %d front %d\n", type, @@ -3467,17 +3476,6 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) return ret; } - -/* - * Free a generically kmalloc'd message. - */ -static void ceph_msg_free(struct ceph_msg *m) -{ - dout("%s %p\n", __func__, m); - kvfree(m->front.iov_base); - kmem_cache_free(ceph_msg_cache, m); -} - static void ceph_msg_release(struct kref *kref) { struct ceph_msg *m = container_of(kref, struct ceph_msg, kref);
Do memory allocation first, so that avoid unnecessary initialization of newly allocated message in error case. Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- net/ceph/messenger.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-)