| Message ID | 1420634785-20791-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 7 January 2015 at 13:46, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/odp_buffer_internal.h | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h > index 60f06c9..c64802b 100644 > --- a/platform/linux-generic/include/odp_buffer_internal.h > +++ b/platform/linux-generic/include/odp_buffer_internal.h > @@ -136,9 +136,6 @@ typedef struct odp_buffer_hdr_t { > typedef struct odp_buffer_hdr_stride { > uint8_t pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))]; > } odp_buffer_hdr_stride; > -/* Ensure next header starts from 8 byte align */ > -_ODP_STATIC_ASSERT((sizeof(odp_buffer_hdr_t) % 8) == 0, > - "ODP_BUFFER_HDR_T__SIZE_ERROR"); Is this the preferred solution? Do we have any alignment requirements on internal headers? Why was this assert added to start with? For the timer/timeout internal header I had to add 32-bit of padding in order to make an equivalent static assert succeed. But perhaps the assert is the problem? > > typedef struct odp_buf_blk_t { > struct odp_buf_blk_t *next; > -- > 2.1.0 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
These asserts were vestigial from the previous linux-generic buffer implementation where the buffer structs were packed within buffer pools and hence needed to be padded to ensure 8-byte alignment. In the current implementation buffers are separately cache-aligned via the stride mechanism so the size of the individual buffer headers is irrelevant. That's a cleaner approach since the effect is maintenance can add whatever it needs to the individual headers for new metadata items, etc. and the alignment is adjusted automatically. In the case of 32-bit implementations the pointers drop from 8 to 4 bytes and this made the individual headers not be multiples of 8, which is why the assert triggered. But as noted, these structures are being cache aligned independent of their individual size so these asserts serve no purpose and can be safely deleted. On Wed, Jan 7, 2015 at 8:13 AM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 7 January 2015 at 13:46, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > platform/linux-generic/include/odp_buffer_internal.h | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/platform/linux-generic/include/odp_buffer_internal.h > b/platform/linux-generic/include/odp_buffer_internal.h > > index 60f06c9..c64802b 100644 > > --- a/platform/linux-generic/include/odp_buffer_internal.h > > +++ b/platform/linux-generic/include/odp_buffer_internal.h > > @@ -136,9 +136,6 @@ typedef struct odp_buffer_hdr_t { > > typedef struct odp_buffer_hdr_stride { > > uint8_t > pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))]; > > } odp_buffer_hdr_stride; > > -/* Ensure next header starts from 8 byte align */ > > -_ODP_STATIC_ASSERT((sizeof(odp_buffer_hdr_t) % 8) == 0, > > - "ODP_BUFFER_HDR_T__SIZE_ERROR"); > Is this the preferred solution? > Do we have any alignment requirements on internal headers? Why was > this assert added to start with? > For the timer/timeout internal header I had to add 32-bit of padding > in order to make an equivalent static assert succeed. But perhaps the > assert is the problem? > > > > > typedef struct odp_buf_blk_t { > > struct odp_buf_blk_t *next; > > -- > > 2.1.0 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 7 January 2015 at 15:19, Bill Fischofer <bill.fischofer@linaro.org> wrote: > These asserts were vestigial from the previous linux-generic buffer > implementation where the buffer structs were packed within buffer pools and > hence needed to be padded to ensure 8-byte alignment. In the current > implementation buffers are separately cache-aligned via the stride mechanism > so the size of the individual buffer headers is irrelevant. That's a cleaner > approach since the effect is maintenance can add whatever it needs to the > individual headers for new metadata items, etc. and the alignment is > adjusted automatically. > > In the case of 32-bit implementations the pointers drop from 8 to 4 bytes > and this made the individual headers not be multiples of 8, which is why the > assert triggered. But as noted, these structures are being cache aligned > independent of their individual size so these asserts serve no purpose and > can be safely deleted. OK. Then I need to do that for the new timer implementation as well. These type of things should have comments so you know why they are needed and when it might no longer be needed. I was just keeping the code building without knowing why this was needed. > > On Wed, Jan 7, 2015 at 8:13 AM, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: >> >> On 7 January 2015 at 13:46, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > --- >> > platform/linux-generic/include/odp_buffer_internal.h | 3 --- >> > 1 file changed, 3 deletions(-) >> > >> > diff --git a/platform/linux-generic/include/odp_buffer_internal.h >> > b/platform/linux-generic/include/odp_buffer_internal.h >> > index 60f06c9..c64802b 100644 >> > --- a/platform/linux-generic/include/odp_buffer_internal.h >> > +++ b/platform/linux-generic/include/odp_buffer_internal.h >> > @@ -136,9 +136,6 @@ typedef struct odp_buffer_hdr_t { >> > typedef struct odp_buffer_hdr_stride { >> > uint8_t >> > pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))]; >> > } odp_buffer_hdr_stride; >> > -/* Ensure next header starts from 8 byte align */ >> > -_ODP_STATIC_ASSERT((sizeof(odp_buffer_hdr_t) % 8) == 0, >> > - "ODP_BUFFER_HDR_T__SIZE_ERROR"); >> Is this the preferred solution? >> Do we have any alignment requirements on internal headers? Why was >> this assert added to start with? >> For the timer/timeout internal header I had to add 32-bit of padding >> in order to make an equivalent static assert succeed. But perhaps the >> assert is the problem? >> >> > >> > typedef struct odp_buf_blk_t { >> > struct odp_buf_blk_t *next; >> > -- >> > 2.1.0 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp > >
The current timer code was updated to use stride mechanism so if you delta'd off of it you should already have it. The only issue here is deleting these stale asserts. On Wed, Jan 7, 2015 at 9:25 AM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 7 January 2015 at 15:19, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > These asserts were vestigial from the previous linux-generic buffer > > implementation where the buffer structs were packed within buffer pools > and > > hence needed to be padded to ensure 8-byte alignment. In the current > > implementation buffers are separately cache-aligned via the stride > mechanism > > so the size of the individual buffer headers is irrelevant. That's a > cleaner > > approach since the effect is maintenance can add whatever it needs to the > > individual headers for new metadata items, etc. and the alignment is > > adjusted automatically. > > > > In the case of 32-bit implementations the pointers drop from 8 to 4 bytes > > and this made the individual headers not be multiples of 8, which is why > the > > assert triggered. But as noted, these structures are being cache aligned > > independent of their individual size so these asserts serve no purpose > and > > can be safely deleted. > OK. Then I need to do that for the new timer implementation as well. > > These type of things should have comments so you know why they are > needed and when it might no longer be needed. I was just keeping the > code building without knowing why this was needed. > > > > > > On Wed, Jan 7, 2015 at 8:13 AM, Ola Liljedahl <ola.liljedahl@linaro.org> > > wrote: > >> > >> On 7 January 2015 at 13:46, Bill Fischofer <bill.fischofer@linaro.org> > >> wrote: > >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > >> > --- > >> > platform/linux-generic/include/odp_buffer_internal.h | 3 --- > >> > 1 file changed, 3 deletions(-) > >> > > >> > diff --git a/platform/linux-generic/include/odp_buffer_internal.h > >> > b/platform/linux-generic/include/odp_buffer_internal.h > >> > index 60f06c9..c64802b 100644 > >> > --- a/platform/linux-generic/include/odp_buffer_internal.h > >> > +++ b/platform/linux-generic/include/odp_buffer_internal.h > >> > @@ -136,9 +136,6 @@ typedef struct odp_buffer_hdr_t { > >> > typedef struct odp_buffer_hdr_stride { > >> > uint8_t > >> > pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))]; > >> > } odp_buffer_hdr_stride; > >> > -/* Ensure next header starts from 8 byte align */ > >> > -_ODP_STATIC_ASSERT((sizeof(odp_buffer_hdr_t) % 8) == 0, > >> > - "ODP_BUFFER_HDR_T__SIZE_ERROR"); > >> Is this the preferred solution? > >> Do we have any alignment requirements on internal headers? Why was > >> this assert added to start with? > >> For the timer/timeout internal header I had to add 32-bit of padding > >> in order to make an equivalent static assert succeed. But perhaps the > >> assert is the problem? > >> > >> > > >> > typedef struct odp_buf_blk_t { > >> > struct odp_buf_blk_t *next; > >> > -- > >> > 2.1.0 > >> > > >> > > >> > _______________________________________________ > >> > lng-odp mailing list > >> > lng-odp@lists.linaro.org > >> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > >
diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h index 60f06c9..c64802b 100644 --- a/platform/linux-generic/include/odp_buffer_internal.h +++ b/platform/linux-generic/include/odp_buffer_internal.h @@ -136,9 +136,6 @@ typedef struct odp_buffer_hdr_t { typedef struct odp_buffer_hdr_stride { uint8_t pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))]; } odp_buffer_hdr_stride; -/* Ensure next header starts from 8 byte align */ -_ODP_STATIC_ASSERT((sizeof(odp_buffer_hdr_t) % 8) == 0, - "ODP_BUFFER_HDR_T__SIZE_ERROR"); typedef struct odp_buf_blk_t { struct odp_buf_blk_t *next;
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/odp_buffer_internal.h | 3 --- 1 file changed, 3 deletions(-)