On Tue, Apr 17, 2018 at 3:13 PM, cgxu519@xxxxxxx <cgxu519@xxxxxxx> wrote:
>
>> 在 2018年4月17日，下午8:46，Ilya Dryomov <idryomov@xxxxxxxxx> 写道：
>>
>> On Tue, Apr 17, 2018 at 12:15 PM, Chengguang Xu <cgxu519@xxxxxxx> wrote:
>>> Do memory allocation first, so that avoid unnecessary
>>> initialization of newly allocated message in error case.
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519@xxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
