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
-----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
>>>
>
