From: Loic Dachary <loic@dachary.org>
To: "Ma, Jianpeng" <jianpeng.ma@intel.com>
Cc: "ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>
Subject: Re: [PATCH 2/3] ec: make use of added aligned buffers
Date: Tue, 16 Sep 2014 09:55:11 +0200 [thread overview]
Message-ID: <5417ECDF.3090706@dachary.org> (raw)
In-Reply-To: <6AA21C22F0A5DA478922644AD2EC308C8C7911@SHSMSX101.ccr.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 9354 bytes --]
On 16/09/2014 08:59, Ma, Jianpeng wrote:
>> -----Original Message-----
>> From: Loic Dachary [mailto:loic@dachary.org]
>> Sent: Tuesday, September 16, 2014 2:47 PM
>> To: Ma, Jianpeng
>> Cc: ceph-devel@vger.kernel.org
>> Subject: Re: [PATCH 2/3] ec: make use of added aligned buffers
>>
>>
>>
>> On 16/09/2014 02:08, Ma, Jianpeng wrote:
>>>> -----Original Message-----
>>>> From: Sage Weil [mailto:sweil@redhat.com]
>>>> Sent: Tuesday, September 16, 2014 8:02 AM
>>>> To: Ma, Jianpeng
>>>> Cc: Loic Dachary; Janne Grunau; ceph-devel@vger.kernel.org
>>>> Subject: RE: [PATCH 2/3] ec: make use of added aligned buffers
>>>>
>>>> On Mon, 15 Sep 2014, Ma, Jianpeng wrote:
>>>>> If we modify bufferlist::c_str() to bufferlist::c_str(bool align).
>>>>> If (align)
>>>>> Posix_memalign(data, CEPH_PAGE_SIZE, len) Else
>>>>> Origin code.
>>>>
>>>> Alignment isn't really a bool, it's an int. c_str(int align=1) ?
>>>>
>>> I mean if we need a align memory after bufferlist::c_str. We can set
>>> the align = true; Now bufferlist::c_str depend on the size when using rebuild if
>> bufferlist have more prt.
>>>
>>> BTW, we also can change the rebuild() && rebuild(ptr). Merge two func into
>> one rebuild(bool align).
>>> By judge the parameter align to alloc align memory or not.
>>>
>>> Or am I missing something?
>>
>> I don't think there is a need for c_str(int align). We make every effort to allocate
>> buffers that are properly aligned. If c_str() does not return an aligned buffer,
>> the proper fix is to align the allocated buffer at the source, not to allocate a
>> new aligned buffer and copy the content of the non aligned buffer into it.
>>
>
>> Do you see a reason why that would be a bad way to deal with alignment ?
> We only guarantee memory align after c_str and don't depend on the previous steps.
This is a benefit indeed: less room for error in general.
> If encode[i] have more ptr suppose they all aligned memory.
> But how to guarantee aligned memory after c_str.
> If bufferlist have more ptr, the aligned memory depend on the size of bufferlist.
The ErasureCode::encode_prepare prepares the allocated buffer so that
*) it holds enough room to contain all chunks
*) c_str() on a chunk boundary will return a pointer that does not require allocating memory
The ErasureCodeJerasure::get_chunk_size function determines the size of the buffer allocated by encode_prepare and further guarantees that each chunk is:
*) size aligned on 16 bytes
*) memory aligned on 16 bytes so that SIMD instructions can use it without moving it
In other words, if c_str() had to re-allocate the buffer because it is not aligned, it would mean that the allocation of the buffer is not done as it should.
Cheers
> Jianpeng
>>
>> Cheers
>>
>>>
>>> Jianpeng
>>>> sage
>>>>
>>>>>
>>>>> I think this is simple and correctly code.
>>>>>
>>>>> Jianpeng
>>>>> Thanks!
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ceph-devel-owner@vger.kernel.org
>>>>>> [mailto:ceph-devel-owner@vger.kernel.org] On Behalf Of Loic Dachary
>>>>>> Sent: Tuesday, September 16, 2014 1:20 AM
>>>>>> To: Janne Grunau; ceph-devel@vger.kernel.org
>>>>>> Subject: Re: [PATCH 2/3] ec: make use of added aligned buffers
>>>>>>
>>>>>> Hi Janne,
>>>>>>
>>>>>> See below:
>>>>>>
>>>>>> On 15/09/2014 17:55, Janne Grunau wrote:
>>>>>>> Requiring page aligned buffers and realigning the input if
>>>>>>> necessary creates measurable oberhead.
>> ceph_erasure_code_benchmark
>>>>>>> is ~30% faster with this change for technique=reed_sol_van,k=2,m=1.
>>>>>>>
>>>>>>> Also prevents a misaligned buffer when
>>>>>>> bufferlist::c_str(bufferlist) has to allocate a new buffer to
>>>>>>> provide continuous one. See bug #9408
>>>>>>>
>>>>>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>>>>>> ---
>>>>>>> src/erasure-code/ErasureCode.cc | 46
>>>>>>> +++++++++++++++++++++++++----------------
>>>>>>> src/erasure-code/ErasureCode.h | 3 ++-
>>>>>>> 2 files changed, 30 insertions(+), 19 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/erasure-code/ErasureCode.cc
>>>>>>> b/src/erasure-code/ErasureCode.cc index 5953f49..078f60b 100644
>>>>>>> --- a/src/erasure-code/ErasureCode.cc
>>>>>>> +++ b/src/erasure-code/ErasureCode.cc
>>>>>>> @@ -54,22 +54,38 @@ int
>>>>>> ErasureCode::minimum_to_decode_with_cost(const
>>>>>>> set<int> &want_to_read, }
>>>>>>>
>>>>>>> int ErasureCode::encode_prepare(const bufferlist &raw,
>>>>>>> - bufferlist *prepared) const
>>>>>>> + map<int, bufferlist> &encoded)
>>>>>> const
>>>>>>> {
>>>>>>> unsigned int k = get_data_chunk_count();
>>>>>>> unsigned int m = get_chunk_count() - k;
>>>>>>> unsigned blocksize = get_chunk_size(raw.length());
>>>>>>> - unsigned padded_length = blocksize * k;
>>>>>>> - *prepared = raw;
>>>>>>> - if (padded_length - raw.length() > 0) {
>>>>>>> - bufferptr pad(padded_length - raw.length());
>>>>>>> - pad.zero();
>>>>>>> - prepared->push_back(pad);
>>>>>>> + unsigned pad_len = blocksize * k - raw.length();
>>>>>>> +
>>>>>>> + bufferlist prepared = raw;
>>>>>>> +
>>>>>>> + if (!prepared.is_aligned()) {
>>>>>>> + prepared.rebuild_aligned();
>>>>>>> + }
>>>>>>> +
>>>>>>> + for (unsigned int i = 0; i < k - !!pad_len; i++) {
>>>>>>> + int chunk_index = chunk_mapping.size() > 0 ? chunk_mapping[i] : i;
>>>>>>> + bufferlist &chunk = encoded[chunk_index];
>>>>>>> + chunk.substr_of(prepared, i * blocksize, blocksize); }
>>>>>>
>>>>>> It is possible for more than one chunk to be padding. It's a border
>>>>>> case but... for instance with alignment = 16, k=12 and in of length
>>>>>> 1550 you end up with two padding chunks because the blocksize is 144.
>>>>>>
>>>>>>> + if (pad_len > 0) {
>>>>>>> + int chunk_index = chunk_mapping.size() > 0 ? chunk_mapping[k
>>>>>>> + - 1] : k -
>>>>>> 1;
>>>>>>> + bufferlist &chunk = encoded[chunk_index];
>>>>>>> + bufferptr padded(buffer::create_aligned(blocksize));
>>>>>>> + raw.copy((k - 1) * blocksize, blocksize - pad_len, padded.c_str());
>>>>>>> + padded.zero(blocksize - pad_len, pad_len);
>>>>>>> + chunk.push_back(padded);
>>>>>>> }
>>>>>>> - unsigned coding_length = blocksize * m;
>>>>>>> - bufferptr coding(buffer::create_page_aligned(coding_length));
>>>>>>> - prepared->push_back(coding);
>>>>>>> - prepared->rebuild_page_aligned();
>>>>>>> + for (unsigned int i = k; i < k + m; i++) {
>>>>>>> + int chunk_index = chunk_mapping.size() > 0 ? chunk_mapping[i] : i;
>>>>>>> + bufferlist &chunk = encoded[chunk_index];
>>>>>>> + chunk.push_back(buffer::create_aligned(blocksize));
>>>>>>> + }
>>>>>>> +
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -80,15 +96,9 @@ int ErasureCode::encode(const set<int>
>>>>>> &want_to_encode,
>>>>>>> unsigned int k = get_data_chunk_count();
>>>>>>> unsigned int m = get_chunk_count() - k;
>>>>>>> bufferlist out;
>>>>>>> - int err = encode_prepare(in, &out);
>>>>>>> + int err = encode_prepare(in, *encoded);
>>>>>>> if (err)
>>>>>>> return err;
>>>>>>> - unsigned blocksize = get_chunk_size(in.length());
>>>>>>> - for (unsigned int i = 0; i < k + m; i++) {
>>>>>>> - int chunk_index = chunk_mapping.size() > 0 ? chunk_mapping[i] : i;
>>>>>>> - bufferlist &chunk = (*encoded)[chunk_index];
>>>>>>> - chunk.substr_of(out, i * blocksize, blocksize);
>>>>>>> - }
>>>>>>> encode_chunks(want_to_encode, encoded);
>>>>>>> for (unsigned int i = 0; i < k + m; i++) {
>>>>>>> if (want_to_encode.count(i) == 0) diff --git
>>>>>>> a/src/erasure-code/ErasureCode.h b/src/erasure-code/ErasureCode.h
>>>>>>> index 7aaea95..62aa383 100644
>>>>>>> --- a/src/erasure-code/ErasureCode.h
>>>>>>> +++ b/src/erasure-code/ErasureCode.h
>>>>>>> @@ -46,7 +46,8 @@ namespace ceph {
>>>>>>> const map<int,
>>>> int>
>>>>>> &available,
>>>>>>> set<int>
>>>> *minimum);
>>>>>>>
>>>>>>> - int encode_prepare(const bufferlist &raw, bufferlist *prepared)
>>>> const;
>>>>>>> + int encode_prepare(const bufferlist &raw,
>>>>>>> + map<int, bufferlist> &encoded) const;
>>>>>>>
>>>>>>> virtual int encode(const set<int> &want_to_encode,
>>>>>>> const bufferlist &in,
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Lo?c Dachary, Artisan Logiciel Libre
>>>>>
>>>>> --
>>>>> 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
>>>>>
>>>>>
>>> --
>>> 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
>>>
>>
>> --
>> Loïc Dachary, Artisan Logiciel Libre
>
> --
> 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
>
--
Loïc Dachary, Artisan Logiciel Libre
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
next prev parent reply other threads:[~2014-09-16 7:55 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-15 15:55 [PATCH 1/3] buffer: add an aligned buffer with less alignment than a page Janne Grunau
2014-09-15 15:55 ` [PATCH 2/3] ec: make use of added aligned buffers Janne Grunau
2014-09-15 17:20 ` Loic Dachary
2014-09-15 23:56 ` Ma, Jianpeng
2014-09-16 0:02 ` Sage Weil
2014-09-16 0:08 ` Ma, Jianpeng
2014-09-16 6:47 ` Loic Dachary
2014-09-16 6:59 ` Ma, Jianpeng
2014-09-16 7:55 ` Loic Dachary [this message]
2014-09-16 8:23 ` Ma, Jianpeng
2014-09-15 15:55 ` [PATCH 3/3] ceph_erasure_code_benchmark: align the encoding input Janne Grunau
2014-09-15 16:46 ` [PATCH 1/3] buffer: add an aligned buffer with less alignment than a page Loic Dachary
2014-09-18 10:33 ` v2 aligned buffer changes for erasure codes Janne Grunau
2014-09-18 10:33 ` [PATCH v2 1/3] buffer: add an aligned buffer with less alignment than a page Janne Grunau
2014-09-18 10:33 ` [PATCH v2 2/3] ec: use 32-byte aligned buffers Janne Grunau
2014-09-19 9:47 ` Loic Dachary
2014-09-18 10:33 ` [PATCH v2 3/3] ceph_erasure_code_benchmark: align the encoding input Janne Grunau
2014-09-18 12:18 ` v2 aligned buffer changes for erasure codes Andreas Joachim Peters
2014-09-18 12:34 ` Andreas Joachim Peters
2014-09-18 12:53 ` Janne Grunau
2014-09-19 9:18 ` Loic Dachary
2014-09-18 12:40 ` Janne Grunau
2014-09-18 13:01 ` Andreas Joachim Peters
2014-09-18 13:23 ` Janne Grunau
2014-09-18 14:47 ` Andreas Joachim Peters
2014-09-29 12:34 ` [PATCH v3 0/4] buffer alignment for erasure code SIMD Janne Grunau
2014-09-29 12:34 ` [PATCH v3 1/4] buffer: add an aligned buffer with less alignment than a page Janne Grunau
2014-09-29 13:12 ` Loic Dachary
2014-10-02 12:09 ` Janne Grunau
2014-09-29 13:27 ` Loic Dachary
2014-10-02 12:12 ` Janne Grunau
2014-10-02 14:17 ` Loic Dachary
2014-09-29 12:34 ` [PATCH v3 2/4] erasure code: use a function for the chunk mapping index Janne Grunau
2014-09-29 12:34 ` [PATCH v3 3/4] erasure code: use 32-byte aligned buffers Janne Grunau
2014-09-29 12:34 ` [PATCH v3 4/4] ceph_erasure_code_benchmark: use 32-byte aligned input Janne Grunau
2014-09-29 13:15 ` [PATCH v3 0/4] buffer alignment for erasure code SIMD Loic Dachary
2014-09-29 15:18 ` Milosz Tanski
2014-09-29 15:24 ` C++11 Sage Weil
2014-09-29 15:44 ` C++11 Milosz Tanski
2014-09-29 17:56 ` C++11 Wido den Hollander
2014-10-02 12:15 ` [PATCH v3 0/4] buffer alignment for erasure code SIMD Janne Grunau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5417ECDF.3090706@dachary.org \
--to=loic@dachary.org \
--cc=ceph-devel@vger.kernel.org \
--cc=jianpeng.ma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox