just to add for some implementation Buffer == data itself (without any
metadata or headroom). So while you work on this, please get us
clarity for such implementations as well.
On Fri, Aug 11, 2017 at 12:36 AM, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 08/10/17 21:41, Liron Himi wrote:
>> Hi,
>>
>> I think it is more than just naming replacement.
>> The odp-pool API state that the alignment is for buffer alignment and not
>> for data. So it seems like a bug, right?
>>
>> Here is a snipped from the API:
>> /** Minimum buffer alignment in bytes. Valid values are
>> powers of two. Use 0 for default alignment.
>> Default will always be a multiple of 8. */
>> uint32_t align;
>> } buf;
>>
>> Regards,
>> Liron
>>
>
>
> Ok, in my understanding this value comes from:
>
> typedef struct odp_pool_capability_t {
> .....
>
> /** Buffer pool capabilities */
> struct {
> /** Maximum buffer data alignment in bytes */
> uint32_t max_align;
>
>
> So it has to be "buffer data alignment". Please give me some time to
> double check with community that we all understand this api definition
> right.
>
> Thank you,
> Maxim.
>
>
>> -----Original Message-----
>> From: Maxim Uvarov [mailto:maxim.uva...@linaro.org]
>> Sent: Thursday, August 10, 2017 19:45
>> To: Liron Himi <lir...@marvell.com>; lng-odp@lists.linaro.org
>> Cc: Petri Savolainen <petri.savolai...@linaro.org>; Elo, Matias (Nokia -
>> FI/Espoo) <matias....@nokia.com>
>> Subject: [EXT] Re: [lng-odp] ODP1.15 buffer alignment issue
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 08/08/17 09:49, Liron Himi wrote:
>>> Hi,
>>>
>>> See comments inline
>>>
>>> Regards,
>>> Liron
>>>
>>> -----Original Message-----
>>> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
>>> Maxim Uvarov
>>> Sent: Monday, August 07, 2017 23:21
>>> To: lng-odp@lists.linaro.org
>>> Subject: Re: [lng-odp] ODP1.15 buffer alignment issue
>>>
>>> it's implementation specific.
>>>
>>> Full code is:
>>>
>>> + offset = pool->headroom;
>>> +
>>> + /* move to correct align */
>>> + while (((uintptr_t)&data[offset]) % pool->align != 0)
>>> + offset++;
>>> <skip>
>>> +
>>> + /* Pointer to data start (of the first segment) */
>>> + buf_hdr->addr[0] = &data[offset];
>>> + /* Store buffer into the global pool */
>>> + ring_enq(ring, mask, (uint32_t)(uintptr_t)buf_hdl);
>>>
>>> If I understood idea right there should be odp specific packet header with
>>> odp fields which is needed to implement api but missing in hardware buffer
>>> field. Then hw buffer which is aligned by default with pointer to data.
>>> Everything depends on implementation and it's not mandatory.
>>> [L.H.] As part of the linux-generic there is a
>>> 'ODP_CONFIG_BUFFER_ALIGN_MIN' which should be used for the buffer alignment.
>>> However, the code above use this value for the data section alignment in
>>> the buffer and not the buffer address alignment.
>>> On ODP1.11 the 'ODP_CONFIG_BUFFER_ALIGN_MIN' was used correctly for buffer
>>> alignment.
>>> In order to continue we need to understand what was the real intention here:
>>> 1. To make sure the data section is aligned?
>>> 2. To make sure the buffer address is aligned? If so, there is a bug in
>>> this code. A possible fix is as follow:
>>> - offset = pool->headroom;
>>> + offset = 0;
>>>
>>> /* move to correct align */
>>> while (((uintptr_t)&data[offset]) % pool->align != 0)
>>> offset++;
>>> + offset += pool->headroom
>>> <skip>
>>> /* Pointer to data start (of the first segment) */
>>> buf_hdr->addr[0] = &data[offset];
>>>
>>> If 1# is required than maybe a 'ODP_CONFIG_DATA_ALIGN_MIN' should be define.
>>>
>>
>> Ah, ok I see. I think in current code we used data align. So that rename to
>> ODP_CONFIG_DATA_ALIGN_MIN is needed. The remain question is should we also
>> align buffers itself.
>>
>> Petri, Matias can you please test with dpdk pktio if buffer alignment
>> improves performance?
>>
>> Maxim.
>>
>>
>>
>>> If your platform reuses this linux-generic file and that alignment does not
>>> work for you then we rework it somehow.
>>>
>>> Bet regards.
>>> Maxim.
>>>
>>>
>>> On 08/07/17 21:58, Bill Fischofer wrote:
>>>> This is part of the latest pool restructure code contributed by Nokia.
>>>> If you'd like to join the ODP public call tomorrow at 15:00 UTC we
>>>> can discuss this then.
>>>>
>>>> On Mon, Aug 7, 2017 at 8:57 AM, Liron Himi <lir...@marvell.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I'm trying to move to odp1.15 and encounter with an buffer alignment
>>>>> issue.
>>>>> It seems that linux-generic implementation make sure that the data
>>>>> address is align with the requested alignment and not the buffer
>>>>> address itself (look at the code sniped below).
>>>>> On ODP1.11 (the current version we use) the buffer was aligned correctly.
>>>>>
>>>>> Is the current behavior is the expected one?
>>>>> Our HW (and probably other HWs) rely on the fact that the buffers
>>>>> are aligned.
>>>>>
>>>>> From odp_pool.c, init_buffers:
>>>>>
>>>>> offset = pool->headroom;
>>>>>
>>>>> /* move to correct align */
>>>>> while (((uintptr_t)&data[offset]) %
>>>>> pool->align != 0)
>>>>> offset++;
>>>>>
>>>>>
>>>>> Regards,
>>>>> Liron
>>>>>
>>>
>>
>
