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