| Message ID | 1461593937-31831-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
I was originally going to require that the max align value be no less than the platform's cache line size. I don't see a use-case for requiring any higher alignment. The maximum practical alignment value would be the align value of the underlying pool since that's the alignment of the entire segment. Silent capping is to avoid extra checks in a fast path, but I have no problem failing on over-aligned requests if that would be seen to be more usable. On Tue, Apr 26, 2016 at 7:00 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT > > Bill Fischofer > > Sent: Monday, April 25, 2016 5:19 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] api: packet: update description for > > odp_packet_align() > > > > Minor rewording for clarity and to specify 0 as a don't care value for > > the align parameter. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > include/odp/api/spec/packet.h | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/include/odp/api/spec/packet.h > b/include/odp/api/spec/packet.h > > index de128df..8eddf2f 100644 > > --- a/include/odp/api/spec/packet.h > > +++ b/include/odp/api/spec/packet.h > > @@ -646,11 +646,11 @@ int odp_packet_rem_data(odp_packet_t *pkt, uint32_t > > offset, uint32_t len); > > * Align packet data > > * > > * Modify packet data alignment so that 'len' bytes between 'offset' and > > - * 'offset' plus 'len' are contiguous in memory and start in minimum > > alignment > > + * 'offset' plus 'len' are contiguous in memory and have a minimum > > alignment > > * of 'align' bytes. > > * > > * A successful operation overwrites the packet handle with a new > handle, > > which > > - * application must use as the reference to the packet instead of the > old > > + * the application must use as the reference to the packet instead of > the > > old > > * handle. Depending on the implementation, the old and new handles may > > be > > * equal. > > * > > @@ -667,7 +667,10 @@ int odp_packet_rem_data(odp_packet_t *pkt, uint32_t > > offset, uint32_t len); > > * @param offset Byte offset of the contiguous area > > * @param len Byte length of the contiguous area (0 ... > > packet_len) > > * @param align Minimum byte alignment of the contiguous area. > > - * Implementation rounds up to nearest power of > two. > > + * Valid values are powers of 2. Use 0 to indicate > > no > > + * special alignment requirement. The > implementation > > may > > + * silently cap the maximum supported alignment, > > which > > + * must be at least 8. > > > Maximum of 8 is too low and silent capping makes it worse. A failure to > align protocol headers correctly would cause bugs. > > Maybe it's better to define that at least 32 byte align is supported and > call fails if align request is too big. We can add capability field for > this later if needed. > > For example, I could image that someone would want to align 16-byte IPv6 > addresses to 16 byte align (or even 32 byte align). CPUs today have 256 bit > (32 byte) vector engines which may benefit greatly from correct data > alignment. > > "Cache alignment > > Aligning data to vector length is always recommended. When using Intel SSE > and Intel SSE2 instructions, loaded data should be aligned to 16 bytes. > Similarly, to achieve best results use Intel AVX instructions on 32-byte > vectors that are 32-byte aligned. ..." > > > https://software.intel.com/en-us/articles/practical-intel-avx-optimization-on-2nd-generation-intel-core-processors > > -Petri > >
v2 posted. On Tue, Apr 26, 2016 at 8:52 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > As discussed on the call. The patch is OK, with these updates > > > > * require that every implementation is able do at least 32 byte align > (instead of 8) > > * call fails when too large align is requested (max align depends on impl > max seg size, which is likely larger than 32 anyway) > > > > -Petri > > > > > > *From:* EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Tuesday, April 26, 2016 3:29 PM > *To:* Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> > *Cc:* lng-odp@lists.linaro.org > *Subject:* Re: [lng-odp] [PATCH] api: packet: update description for > odp_packet_align() > > > > I was originally going to require that the max align value be no less than > the platform's cache line size. I don't see a use-case for requiring any > higher alignment. > > > > The maximum practical alignment value would be the align value of the > underlying pool since that's the alignment of the entire segment. Silent > capping is to avoid extra checks in a fast path, but I have no problem > failing on over-aligned requests if that would be seen to be more usable. > > > > On Tue, Apr 26, 2016 at 7:00 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT > > Bill Fischofer > > Sent: Monday, April 25, 2016 5:19 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] api: packet: update description for > > odp_packet_align() > > > > Minor rewording for clarity and to specify 0 as a don't care value for > > the align parameter. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > include/odp/api/spec/packet.h | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/include/odp/api/spec/packet.h > b/include/odp/api/spec/packet.h > > index de128df..8eddf2f 100644 > > --- a/include/odp/api/spec/packet.h > > +++ b/include/odp/api/spec/packet.h > > @@ -646,11 +646,11 @@ int odp_packet_rem_data(odp_packet_t *pkt, uint32_t > > offset, uint32_t len); > > * Align packet data > > * > > * Modify packet data alignment so that 'len' bytes between 'offset' and > > - * 'offset' plus 'len' are contiguous in memory and start in minimum > > alignment > > + * 'offset' plus 'len' are contiguous in memory and have a minimum > > alignment > > * of 'align' bytes. > > * > > * A successful operation overwrites the packet handle with a new > handle, > > which > > - * application must use as the reference to the packet instead of the > old > > + * the application must use as the reference to the packet instead of > the > > old > > * handle. Depending on the implementation, the old and new handles may > > be > > * equal. > > * > > @@ -667,7 +667,10 @@ int odp_packet_rem_data(odp_packet_t *pkt, uint32_t > > offset, uint32_t len); > > * @param offset Byte offset of the contiguous area > > * @param len Byte length of the contiguous area (0 ... > > packet_len) > > * @param align Minimum byte alignment of the contiguous area. > > - * Implementation rounds up to nearest power of > two. > > + * Valid values are powers of 2. Use 0 to indicate > > no > > + * special alignment requirement. The > implementation > > may > > + * silently cap the maximum supported alignment, > > which > > + * must be at least 8. > > Maximum of 8 is too low and silent capping makes it worse. A failure to > align protocol headers correctly would cause bugs. > > Maybe it's better to define that at least 32 byte align is supported and > call fails if align request is too big. We can add capability field for > this later if needed. > > For example, I could image that someone would want to align 16-byte IPv6 > addresses to 16 byte align (or even 32 byte align). CPUs today have 256 bit > (32 byte) vector engines which may benefit greatly from correct data > alignment. > > "Cache alignment > > Aligning data to vector length is always recommended. When using Intel SSE > and Intel SSE2 instructions, loaded data should be aligned to 16 bytes. > Similarly, to achieve best results use Intel AVX instructions on 32-byte > vectors that are 32-byte aligned. ..." > > > https://software.intel.com/en-us/articles/practical-intel-avx-optimization-on-2nd-generation-intel-core-processors > > -Petri > > >
diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h index de128df..8eddf2f 100644 --- a/include/odp/api/spec/packet.h +++ b/include/odp/api/spec/packet.h @@ -646,11 +646,11 @@ int odp_packet_rem_data(odp_packet_t *pkt, uint32_t offset, uint32_t len); * Align packet data * * Modify packet data alignment so that 'len' bytes between 'offset' and - * 'offset' plus 'len' are contiguous in memory and start in minimum alignment + * 'offset' plus 'len' are contiguous in memory and have a minimum alignment * of 'align' bytes. * * A successful operation overwrites the packet handle with a new handle, which - * application must use as the reference to the packet instead of the old + * the application must use as the reference to the packet instead of the old * handle. Depending on the implementation, the old and new handles may be * equal. * @@ -667,7 +667,10 @@ int odp_packet_rem_data(odp_packet_t *pkt, uint32_t offset, uint32_t len); * @param offset Byte offset of the contiguous area * @param len Byte length of the contiguous area (0 ... packet_len) * @param align Minimum byte alignment of the contiguous area. - * Implementation rounds up to nearest power of two. + * Valid values are powers of 2. Use 0 to indicate no + * special alignment requirement. The implementation may + * silently cap the maximum supported alignment, which + * must be at least 8. * * @retval 0 Operation successful, old pointers remain valid * @retval >0 Operation successful, old pointers need to be updated
Minor rewording for clarity and to specify 0 as a don't care value for the align parameter. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- include/odp/api/spec/packet.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)