| Message ID | 1423869249-504-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
V1 had it this way but I changed that in V2 to put type first as the rest is a variable-length structure (depending on type) so it seemed to make more sense to have the key for decoding the rest first. Alignment is not an issue here. It's not a "may have" since the actual structure layouts we're defining here are part of the ODP API and are not subject to per-implementation differences. On Tue, Feb 17, 2015 at 7:32 PM, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer > > Sent: Saturday, February 14, 2015 1:14 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv2 3/4] api: pools: normalize odp_pool_param_t > > layout > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > include/odp/api/pool.h | 24 +++++++++--------------- > > 1 file changed, 9 insertions(+), 15 deletions(-) > > > > diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h > > index 66dc70e..b15ddd5 100644 > > --- a/include/odp/api/pool.h > > +++ b/include/odp/api/pool.h > > @@ -47,8 +47,10 @@ extern "C" { > > * Used to communicate pool creation options. > > */ > > typedef struct odp_pool_param_t { > > + int type; /**< Pool type */ > > A minor thing. I'd keep "type" after the union in this struct. The > union may have larger alignment requirement than int (depending on > compiler) > and in that case would force padding here (between int and union). > > Otherwise OK. > > -Petri > > > > union { > > struct { > > + uint32_t num; /**< Number of buffers in the pool > */ > > uint32_t size; /**< Buffer size in bytes. The > > maximum number of bytes > > application will store in each > > @@ -58,20 +60,11 @@ typedef struct odp_pool_param_t { > > Use 0 for default alignment. > > Default will always be a > multiple > > of 8. */ > > - uint32_t num; /**< Number of buffers in the pool > */ > > } buf; > > struct { > > - uint32_t seg_len; /**< Minimum number of packet > data > > - bytes that are stored in > the > > - first segment of a packet. > > - The maximum value is > defined by > > - > ODP_CONFIG_PACKET_SEG_LEN_MAX. > > - Use 0 for default. */ > > - uint32_t __res1; /* Keep struct identical to > buf, > > - until implementation is > fixed */ > > uint32_t num; /**< The number of packets > that the > > pool must provide that are > > - packet lenght 'len' bytes > or > > + packet length 'len' bytes > or > > smaller. */ > > uint32_t len; /**< Minimum packet length > that the > > pool must provide 'num' > > @@ -80,16 +73,17 @@ typedef struct odp_pool_param_t { > > packets are larger than > 'len'. > > Use 0 for default. > > */ > > + uint32_t seg_len; /**< Minimum number of packet > data > > + bytes that are stored in > the > > + first segment of a packet. > > + The maximum value is > defined by > > + > ODP_CONFIG_PACKET_SEG_LEN_MAX. > > + Use 0 for default. */ > > } pkt; > > struct { > > - uint32_t __res1; /* Keep struct identical to buf, > */ > > - uint32_t __res2; /* until pool implementation is > fixed*/ > > uint32_t num; /**< Number of timeouts in the > pool */ > > } tmo; > > }; > > - > > - int type; /**< Pool type */ > > - > > } odp_pool_param_t; > > > > /** Packet pool*/ > > -- > > 2.1.0 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 17 February 2015 at 19:43, Bill Fischofer <bill.fischofer@linaro.org> wrote: > V1 had it this way but I changed that in V2 to put type first as the rest is > a variable-length structure (depending on type) so it seemed to make more > sense to have the key for decoding the rest first. Alignment is not an > issue here. It's not a "may have" since the actual structure layouts we're > defining here are part of the ODP API and are not subject to > per-implementation differences. Then we should be extra careful with the struct layout so that it does not change with different compilers or compiler flags. All fields on natural alignment, explicit padding if there is a hole etc. -- Ola > > On Tue, Feb 17, 2015 at 7:32 PM, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: >> >> >> >> > -----Original Message----- >> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer >> > Sent: Saturday, February 14, 2015 1:14 AM >> > To: lng-odp@lists.linaro.org >> > Subject: [lng-odp] [PATCHv2 3/4] api: pools: normalize odp_pool_param_t >> > layout >> > >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > --- >> > include/odp/api/pool.h | 24 +++++++++--------------- >> > 1 file changed, 9 insertions(+), 15 deletions(-) >> > >> > diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h >> > index 66dc70e..b15ddd5 100644 >> > --- a/include/odp/api/pool.h >> > +++ b/include/odp/api/pool.h >> > @@ -47,8 +47,10 @@ extern "C" { >> > * Used to communicate pool creation options. >> > */ >> > typedef struct odp_pool_param_t { >> > + int type; /**< Pool type */ >> >> A minor thing. I'd keep "type" after the union in this struct. The >> union may have larger alignment requirement than int (depending on >> compiler) >> and in that case would force padding here (between int and union). >> >> Otherwise OK. >> >> -Petri >> >> >> > union { >> > struct { >> > + uint32_t num; /**< Number of buffers in the pool >> > */ >> > uint32_t size; /**< Buffer size in bytes. The >> > maximum number of bytes >> > application will store in >> > each >> > @@ -58,20 +60,11 @@ typedef struct odp_pool_param_t { >> > Use 0 for default alignment. >> > Default will always be a >> > multiple >> > of 8. */ >> > - uint32_t num; /**< Number of buffers in the pool >> > */ >> > } buf; >> > struct { >> > - uint32_t seg_len; /**< Minimum number of packet >> > data >> > - bytes that are stored in >> > the >> > - first segment of a >> > packet. >> > - The maximum value is >> > defined by >> > - >> > ODP_CONFIG_PACKET_SEG_LEN_MAX. >> > - Use 0 for default. */ >> > - uint32_t __res1; /* Keep struct identical to >> > buf, >> > - until implementation is >> > fixed */ >> > uint32_t num; /**< The number of packets >> > that the >> > pool must provide that >> > are >> > - packet lenght 'len' bytes >> > or >> > + packet length 'len' bytes >> > or >> > smaller. */ >> > uint32_t len; /**< Minimum packet length >> > that the >> > pool must provide 'num' >> > @@ -80,16 +73,17 @@ typedef struct odp_pool_param_t { >> > packets are larger than >> > 'len'. >> > Use 0 for default. >> > */ >> > + uint32_t seg_len; /**< Minimum number of packet >> > data >> > + bytes that are stored in >> > the >> > + first segment of a >> > packet. >> > + The maximum value is >> > defined by >> > + >> > ODP_CONFIG_PACKET_SEG_LEN_MAX. >> > + Use 0 for default. */ >> > } pkt; >> > struct { >> > - uint32_t __res1; /* Keep struct identical to buf, >> > */ >> > - uint32_t __res2; /* until pool implementation is >> > fixed*/ >> > uint32_t num; /**< Number of timeouts in the >> > pool */ >> > } tmo; >> > }; >> > - >> > - int type; /**< Pool type */ >> > - >> > } odp_pool_param_t; >> > >> > /** Packet pool*/ >> > -- >> > 2.1.0 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
That would make trying to create an that contains any structs ABI problematic, however I'm not sure it's relevant here since v1.0 only has an API and each compiler will be consistent with itself. in its choice of mappings. On Wed, Feb 18, 2015 at 2:00 AM, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > Structure alignment and padding is compiler specific. I remember seeing > compiler aligning all structs into 8-byte alignment, also when all members > are <8 bytes. > > > > -Petri > > > > > > > > *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Tuesday, February 17, 2015 8:44 PM > *To:* Savolainen, Petri (NSN - FI/Espoo) > *Cc:* lng-odp@lists.linaro.org > *Subject:* Re: [lng-odp] [PATCHv2 3/4] api: pools: normalize > odp_pool_param_t layout > > > > V1 had it this way but I changed that in V2 to put type first as the rest > is a variable-length structure (depending on type) so it seemed to make > more sense to have the key for decoding the rest first. Alignment is not > an issue here. It's not a "may have" since the actual structure layouts > we're defining here are part of the ODP API and are not subject to > per-implementation differences. > > > > On Tue, Feb 17, 2015 at 7:32 PM, Savolainen, Petri (NSN - FI/Espoo) < > petri.savolainen@nsn.com> wrote: > > > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer > > Sent: Saturday, February 14, 2015 1:14 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv2 3/4] api: pools: normalize odp_pool_param_t > > layout > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > include/odp/api/pool.h | 24 +++++++++--------------- > > 1 file changed, 9 insertions(+), 15 deletions(-) > > > > diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h > > index 66dc70e..b15ddd5 100644 > > --- a/include/odp/api/pool.h > > +++ b/include/odp/api/pool.h > > @@ -47,8 +47,10 @@ extern "C" { > > * Used to communicate pool creation options. > > */ > > typedef struct odp_pool_param_t { > > + int type; /**< Pool type */ > > A minor thing. I'd keep "type" after the union in this struct. The > union may have larger alignment requirement than int (depending on > compiler) > and in that case would force padding here (between int and union). > > Otherwise OK. > > -Petri > > > > > union { > > struct { > > + uint32_t num; /**< Number of buffers in the pool > */ > > uint32_t size; /**< Buffer size in bytes. The > > maximum number of bytes > > application will store in each > > @@ -58,20 +60,11 @@ typedef struct odp_pool_param_t { > > Use 0 for default alignment. > > Default will always be a > multiple > > of 8. */ > > - uint32_t num; /**< Number of buffers in the pool > */ > > } buf; > > struct { > > - uint32_t seg_len; /**< Minimum number of packet > data > > - bytes that are stored in > the > > - first segment of a packet. > > - The maximum value is > defined by > > - > ODP_CONFIG_PACKET_SEG_LEN_MAX. > > - Use 0 for default. */ > > - uint32_t __res1; /* Keep struct identical to > buf, > > - until implementation is > fixed */ > > uint32_t num; /**< The number of packets > that the > > pool must provide that are > > - packet lenght 'len' bytes > or > > + packet length 'len' bytes > or > > smaller. */ > > uint32_t len; /**< Minimum packet length > that the > > pool must provide 'num' > > @@ -80,16 +73,17 @@ typedef struct odp_pool_param_t { > > packets are larger than > 'len'. > > Use 0 for default. > > */ > > + uint32_t seg_len; /**< Minimum number of packet > data > > + bytes that are stored in > the > > + first segment of a packet. > > + The maximum value is > defined by > > + > ODP_CONFIG_PACKET_SEG_LEN_MAX. > > + Use 0 for default. */ > > } pkt; > > struct { > > - uint32_t __res1; /* Keep struct identical to buf, > */ > > - uint32_t __res2; /* until pool implementation is > fixed*/ > > uint32_t num; /**< Number of timeouts in the > pool */ > > } tmo; > > }; > > - > > - int type; /**< Pool type */ > > - > > } odp_pool_param_t; > > > > /** Packet pool*/ > > -- > > 2.1.0 > > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp > > >
On 18 February 2015 at 12:56, Bill Fischofer <bill.fischofer@linaro.org> wrote: > That would make trying to create an that contains any structs ABI > problematic, however I'm not sure it's relevant here since v1.0 only has an > API and each compiler will be consistent with itself. in its choice of > mappings. When is the ODP deb package supposed to come live? With the deb package, you have no idea how the ODP library was built. And what's in the deb package? Header files + linux-generic? Maybe we should only have deb source packages. > > On Wed, Feb 18, 2015 at 2:00 AM, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: >> >> Structure alignment and padding is compiler specific. I remember seeing >> compiler aligning all structs into 8-byte alignment, also when all members >> are <8 bytes. >> >> >> >> -Petri >> >> >> >> >> >> >> >> From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org] >> Sent: Tuesday, February 17, 2015 8:44 PM >> To: Savolainen, Petri (NSN - FI/Espoo) >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv2 3/4] api: pools: normalize >> odp_pool_param_t layout >> >> >> >> V1 had it this way but I changed that in V2 to put type first as the rest >> is a variable-length structure (depending on type) so it seemed to make more >> sense to have the key for decoding the rest first. Alignment is not an >> issue here. It's not a "may have" since the actual structure layouts we're >> defining here are part of the ODP API and are not subject to >> per-implementation differences. >> >> >> >> On Tue, Feb 17, 2015 at 7:32 PM, Savolainen, Petri (NSN - FI/Espoo) >> <petri.savolainen@nsn.com> wrote: >> >> >> >> > -----Original Message----- >> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer >> > Sent: Saturday, February 14, 2015 1:14 AM >> > To: lng-odp@lists.linaro.org >> > Subject: [lng-odp] [PATCHv2 3/4] api: pools: normalize odp_pool_param_t >> > layout >> > >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > --- >> > include/odp/api/pool.h | 24 +++++++++--------------- >> > 1 file changed, 9 insertions(+), 15 deletions(-) >> > >> > diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h >> > index 66dc70e..b15ddd5 100644 >> > --- a/include/odp/api/pool.h >> > +++ b/include/odp/api/pool.h >> > @@ -47,8 +47,10 @@ extern "C" { >> > * Used to communicate pool creation options. >> > */ >> > typedef struct odp_pool_param_t { >> > + int type; /**< Pool type */ >> >> A minor thing. I'd keep "type" after the union in this struct. The >> union may have larger alignment requirement than int (depending on >> compiler) >> and in that case would force padding here (between int and union). >> >> Otherwise OK. >> >> -Petri >> >> >> >> > union { >> > struct { >> > + uint32_t num; /**< Number of buffers in the pool >> > */ >> > uint32_t size; /**< Buffer size in bytes. The >> > maximum number of bytes >> > application will store in >> > each >> > @@ -58,20 +60,11 @@ typedef struct odp_pool_param_t { >> > Use 0 for default alignment. >> > Default will always be a >> > multiple >> > of 8. */ >> > - uint32_t num; /**< Number of buffers in the pool >> > */ >> > } buf; >> > struct { >> > - uint32_t seg_len; /**< Minimum number of packet >> > data >> > - bytes that are stored in >> > the >> > - first segment of a >> > packet. >> > - The maximum value is >> > defined by >> > - >> > ODP_CONFIG_PACKET_SEG_LEN_MAX. >> > - Use 0 for default. */ >> > - uint32_t __res1; /* Keep struct identical to >> > buf, >> > - until implementation is >> > fixed */ >> > uint32_t num; /**< The number of packets >> > that the >> > pool must provide that >> > are >> > - packet lenght 'len' bytes >> > or >> > + packet length 'len' bytes >> > or >> > smaller. */ >> > uint32_t len; /**< Minimum packet length >> > that the >> > pool must provide 'num' >> > @@ -80,16 +73,17 @@ typedef struct odp_pool_param_t { >> > packets are larger than >> > 'len'. >> > Use 0 for default. >> > */ >> > + uint32_t seg_len; /**< Minimum number of packet >> > data >> > + bytes that are stored in >> > the >> > + first segment of a >> > packet. >> > + The maximum value is >> > defined by >> > + >> > ODP_CONFIG_PACKET_SEG_LEN_MAX. >> > + Use 0 for default. */ >> > } pkt; >> > struct { >> > - uint32_t __res1; /* Keep struct identical to buf, >> > */ >> > - uint32_t __res2; /* until pool implementation is >> > fixed*/ >> > uint32_t num; /**< Number of timeouts in the >> > pool */ >> > } tmo; >> > }; >> > - >> > - int type; /**< Pool type */ >> > - >> > } odp_pool_param_t; >> > >> > /** Packet pool*/ >> > -- >> > 2.1.0 >> > >> > >> >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h index 66dc70e..b15ddd5 100644 --- a/include/odp/api/pool.h +++ b/include/odp/api/pool.h @@ -47,8 +47,10 @@ extern "C" { * Used to communicate pool creation options. */ typedef struct odp_pool_param_t { + int type; /**< Pool type */ union { struct { + uint32_t num; /**< Number of buffers in the pool */ uint32_t size; /**< Buffer size in bytes. The maximum number of bytes application will store in each @@ -58,20 +60,11 @@ typedef struct odp_pool_param_t { Use 0 for default alignment. Default will always be a multiple of 8. */ - uint32_t num; /**< Number of buffers in the pool */ } buf; struct { - uint32_t seg_len; /**< Minimum number of packet data - bytes that are stored in the - first segment of a packet. - The maximum value is defined by - ODP_CONFIG_PACKET_SEG_LEN_MAX. - Use 0 for default. */ - uint32_t __res1; /* Keep struct identical to buf, - until implementation is fixed */ uint32_t num; /**< The number of packets that the pool must provide that are - packet lenght 'len' bytes or + packet length 'len' bytes or smaller. */ uint32_t len; /**< Minimum packet length that the pool must provide 'num' @@ -80,16 +73,17 @@ typedef struct odp_pool_param_t { packets are larger than 'len'. Use 0 for default. */ + uint32_t seg_len; /**< Minimum number of packet data + bytes that are stored in the + first segment of a packet. + The maximum value is defined by + ODP_CONFIG_PACKET_SEG_LEN_MAX. + Use 0 for default. */ } pkt; struct { - uint32_t __res1; /* Keep struct identical to buf, */ - uint32_t __res2; /* until pool implementation is fixed*/ uint32_t num; /**< Number of timeouts in the pool */ } tmo; }; - - int type; /**< Pool type */ - } odp_pool_param_t; /** Packet pool*/
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- include/odp/api/pool.h | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)