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

Reply via email to