> 在 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.
Thanks,
Chengguang.
--
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
