[lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once
Signed-off-by: Nicolas Morey-Chaisemartin Reviewed-by: Petri Savolainen --- include/odp/api/buffer.h | 26 ++ 1 file changed, 26 insertions(+) diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h index aea273f..6631f47 100644 --- a/include/odp/api/buffer.h +++ b/include/odp/api/buffer.h @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); odp_buffer_t odp_buffer_alloc(odp_pool_t pool); /** + * Allocate multiple buffers + + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers from a pool + * + * @param pool Pool handle + * @param[out] buf Array of buffer handles for output + * @param num Maximum number of buffers to allocate + * + * @return Number of buffers actually allocated (0 ... num) + * @retval <0 on failure + */ +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int num); + +/** * Buffer free * * @param buf Buffer handle @@ -113,6 +127,18 @@ odp_buffer_t odp_buffer_alloc(odp_pool_t pool); void odp_buffer_free(odp_buffer_t buf); /** + * Free multiple buffers + * + * Otherwise like odp_buffer_free(), but frees multiple buffers + * to their originating pools. + * + * @param bufArray of buffer handles + * @param numNumber of buffer handles to free + * + */ +void odp_buffer_free_multi(const odp_buffer_t buf[], int num); + +/** * Print buffer metadata to STDOUT * * @param buf Buffer handle -- 2.6.1.3.g8d02103 ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once
Hi, I know it's a late cry, but I've found two problems while implementing this for ODP-DPDK, and the apply for odp_packet_alloc_multi() as well. See inline: On 28/10/15 15:31, Nicolas Morey-Chaisemartin wrote: Signed-off-by: Nicolas Morey-Chaisemartin Reviewed-by: Petri Savolainen --- include/odp/api/buffer.h | 26 ++ 1 file changed, 26 insertions(+) diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h index aea273f..6631f47 100644 --- a/include/odp/api/buffer.h +++ b/include/odp/api/buffer.h @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); odp_buffer_t odp_buffer_alloc(odp_pool_t pool); /** + * Allocate multiple buffers + + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers from a pool + * + * @param pool Pool handle + * @param[out] buf Array of buffer handles for output + * @param num Maximum number of buffers to allocate + * + * @return Number of buffers actually allocated (0 ... num) + * @retval <0 on failure These two lines contradicts each other: the first says we should return how much we managed to allocate, and we will always allocate at least 0 buffers. The second says in case of error a negative value, but even if we fail straight away, the first line commands us to return 0. Moreover, if we managed to allocate at least some of the buffers and then we return a negative value due to some error (e.g. ENOMEM, or anything else), the application won't know if there is any buffer to use in the array. Either we should clarify that the platform should release them in case of an error, or (and I think that would be the better thing to do) we should return how much we allocated. Altogether, I think we should remove the "<0 on failure" statement and set the errno instead. If you agree, Nicolas, would you submit a patch for that? + */ +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int num); 'num' should be unsigned, a negative number doesn't make sense in this case. Although changing this breaks the ABI, so I'm not sure if we should fix this or just leave it. ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once
On Thu, Dec 3, 2015 at 8:30 AM, Zoltan Kiss wrote: > Hi, > > I know it's a late cry, but I've found two problems while implementing > this for ODP-DPDK, and the apply for odp_packet_alloc_multi() as well. See > inline: > > On 28/10/15 15:31, Nicolas Morey-Chaisemartin wrote: > >> Signed-off-by: Nicolas Morey-Chaisemartin >> Reviewed-by: Petri Savolainen >> --- >> include/odp/api/buffer.h | 26 ++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >> index aea273f..6631f47 100644 >> --- a/include/odp/api/buffer.h >> +++ b/include/odp/api/buffer.h >> @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); >> odp_buffer_t odp_buffer_alloc(odp_pool_t pool); >> >> /** >> + * Allocate multiple buffers >> + >> + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers >> from a pool >> + * >> + * @param pool Pool handle >> + * @param[out] buf Array of buffer handles for output >> + * @param num Maximum number of buffers to allocate >> + * >> + * @return Number of buffers actually allocated (0 ... num) >> + * @retval <0 on failure >> > > These two lines contradicts each other: the first says we should return > how much we managed to allocate, and we will always allocate at least 0 > buffers. > The second says in case of error a negative value, but even if we fail > straight away, the first line commands us to return 0. Moreover, if we > managed to allocate at least some of the buffers and then we return a > negative value due to some error (e.g. ENOMEM, or anything else), the > application won't know if there is any buffer to use in the array. Either > we should clarify that the platform should release them in case of an > error, or (and I think that would be the better thing to do) we should > return how much we allocated. > Altogether, I think we should remove the "<0 on failure" statement and set > the errno instead. If you agree, Nicolas, would you submit a patch for that? This is just being consistent with other multi() APIs like odp_queue_enq_multi(). I believe we had this discussion surrounding those APIs and decided to keep these semantics. Negative RCs say the error is permanent. An RC of 0 would indicate that nothing was done however the operation is retryable. We don't actually make use of that RC for now, so perhaps the change should be to say the RC is 1..num rather than 0..num. > > + */ >> +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int num); >> > > 'num' should be unsigned, a negative number doesn't make sense in this > case. Although changing this breaks the ABI, so I'm not sure if we should > fix this or just leave it. Again this is copied directly from the other multi() APIs. Agree they probably should be unsigned but I don't think it's a big deal since in the unlikely event a negative number was supplied you wouldn't get any buffers anyway. By the same argument specifying 0 here wouldn't make sense semantically and yet that's a valid int and unsigned. > > > ___ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once
> -Original Message- > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of EXT > Zoltan Kiss > Sent: Thursday, December 03, 2015 4:30 PM > To: Nicolas Morey-Chaisemartin; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to > alloc/free multiple buffers at once > > Hi, > > I know it's a late cry, but I've found two problems while implementing > this for ODP-DPDK, and the apply for odp_packet_alloc_multi() as well. > See inline: > > On 28/10/15 15:31, Nicolas Morey-Chaisemartin wrote: > > Signed-off-by: Nicolas Morey-Chaisemartin > > Reviewed-by: Petri Savolainen > > --- > > include/odp/api/buffer.h | 26 ++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h > > index aea273f..6631f47 100644 > > --- a/include/odp/api/buffer.h > > +++ b/include/odp/api/buffer.h > > @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); > > odp_buffer_t odp_buffer_alloc(odp_pool_t pool); > > > > /** > > + * Allocate multiple buffers > > + > > + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers > from a pool > > + * > > + * @param pool Pool handle > > + * @param[out] buf Array of buffer handles for output > > + * @param num Maximum number of buffers to allocate > > + * > > + * @return Number of buffers actually allocated (0 ... num) > > + * @retval <0 on failure > > These two lines contradicts each other: the first says we should return > how much we managed to allocate, and we will always allocate at least 0 > buffers. > The second says in case of error a negative value, but even if we fail > straight away, the first line commands us to return 0. Moreover, if we > managed to allocate at least some of the buffers and then we return a > negative value due to some error (e.g. ENOMEM, or anything else), the > application won't know if there is any buffer to use in the array. > Either we should clarify that the platform should release them in case > of an error, or (and I think that would be the better thing to do) we > should return how much we allocated. > Altogether, I think we should remove the "<0 on failure" statement and > set the errno instead. If you agree, Nicolas, would you submit a patch > for that? > This is inline with other _multi() calls. 0 packets/buffers/events processed is a valid outcome. Pool was empty, queue was full, etc. Negative is a real failure: bad handle, etc. In case of failure, the call must not perform the action (e.g. half way). Either 0...num packets were processed, or nothing was processed in case of failure. > > + */ > > +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int > num); > > 'num' should be unsigned, a negative number doesn't make sense in this > case. Although changing this breaks the ABI, so I'm not sure if we > should fix this or just leave it. This is inline with other multi calls. Num is int since return value is int. User can compare (without warnings) if (odp_buffer_alloc_multi(, num) == num) // Success... -Petri > > ___ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once
On 03/12/15 15:02, Savolainen, Petri (Nokia - FI/Espoo) wrote: -Original Message- From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of EXT Zoltan Kiss Sent: Thursday, December 03, 2015 4:30 PM To: Nicolas Morey-Chaisemartin; lng-odp@lists.linaro.org Subject: Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once Hi, I know it's a late cry, but I've found two problems while implementing this for ODP-DPDK, and the apply for odp_packet_alloc_multi() as well. See inline: On 28/10/15 15:31, Nicolas Morey-Chaisemartin wrote: Signed-off-by: Nicolas Morey-Chaisemartin Reviewed-by: Petri Savolainen --- include/odp/api/buffer.h | 26 ++ 1 file changed, 26 insertions(+) diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h index aea273f..6631f47 100644 --- a/include/odp/api/buffer.h +++ b/include/odp/api/buffer.h @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); odp_buffer_t odp_buffer_alloc(odp_pool_t pool); /** + * Allocate multiple buffers + + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers from a pool + * + * @param pool Pool handle + * @param[out] buf Array of buffer handles for output + * @param num Maximum number of buffers to allocate + * + * @return Number of buffers actually allocated (0 ... num) + * @retval <0 on failure These two lines contradicts each other: the first says we should return how much we managed to allocate, and we will always allocate at least 0 buffers. The second says in case of error a negative value, but even if we fail straight away, the first line commands us to return 0. Moreover, if we managed to allocate at least some of the buffers and then we return a negative value due to some error (e.g. ENOMEM, or anything else), the application won't know if there is any buffer to use in the array. Either we should clarify that the platform should release them in case of an error, or (and I think that would be the better thing to do) we should return how much we allocated. Altogether, I think we should remove the "<0 on failure" statement and set the errno instead. If you agree, Nicolas, would you submit a patch for that? This is inline with other _multi() calls. 0 packets/buffers/events processed is a valid outcome. Pool was empty, queue was full, etc. Negative is a real failure: bad handle, etc. In case of failure, the call must not perform the action (e.g. half way). Either 0...num packets were processed, or nothing was processed in case of failure. I see. Maybe it would worth to add that ENOMEM-like cases are not failures in this case, because sometimes it is interpreted as failure. + */ +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int num); 'num' should be unsigned, a negative number doesn't make sense in this case. Although changing this breaks the ABI, so I'm not sure if we should fix this or just leave it. This is inline with other multi calls. Num is int since return value is int. User can compare (without warnings) Makes sense if (odp_buffer_alloc_multi(, num) == num) // Success... -Petri ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
Re: [lng-odp] [API-NEXTv6 1/7] api: buffer: add functions to alloc/free multiple buffers at once
On 3 December 2015 at 16:01, Bill Fischofer wrote: > > > On Thu, Dec 3, 2015 at 8:30 AM, Zoltan Kiss > wrote: > >> Hi, >> >> I know it's a late cry, but I've found two problems while implementing >> this for ODP-DPDK, and the apply for odp_packet_alloc_multi() as well. See >> inline: >> >> On 28/10/15 15:31, Nicolas Morey-Chaisemartin wrote: >> >>> Signed-off-by: Nicolas Morey-Chaisemartin >>> Reviewed-by: Petri Savolainen >>> --- >>> include/odp/api/buffer.h | 26 ++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >>> index aea273f..6631f47 100644 >>> --- a/include/odp/api/buffer.h >>> +++ b/include/odp/api/buffer.h >>> @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); >>> odp_buffer_t odp_buffer_alloc(odp_pool_t pool); >>> >>> /** >>> + * Allocate multiple buffers >>> + >>> + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers >>> from a pool >>> + * >>> + * @param pool Pool handle >>> + * @param[out] buf Array of buffer handles for output >>> + * @param num Maximum number of buffers to allocate >>> + * >>> + * @return Number of buffers actually allocated (0 ... num) >>> + * @retval <0 on failure >>> >> >> These two lines contradicts each other: the first says we should return >> how much we managed to allocate, and we will always allocate at least 0 >> buffers. >> The second says in case of error a negative value, but even if we fail >> straight away, the first line commands us to return 0. Moreover, if we >> managed to allocate at least some of the buffers and then we return a >> negative value due to some error (e.g. ENOMEM, or anything else), the >> application won't know if there is any buffer to use in the array. Either >> we should clarify that the platform should release them in case of an >> error, or (and I think that would be the better thing to do) we should >> return how much we allocated. >> Altogether, I think we should remove the "<0 on failure" statement and >> set the errno instead. If you agree, Nicolas, would you submit a patch for >> that? > > > This is just being consistent with other multi() APIs like > odp_queue_enq_multi(). I believe we had this discussion surrounding those > APIs and decided to keep these semantics. Negative RCs say the error is > permanent. An RC of 0 would indicate that nothing was done however the > operation is retryable. We don't actually make use of that RC for now, > "We don't actually make use of that RC for now" I don't understand this sentence. > so perhaps the change should be to say the RC is 1..num rather than 0..num. > Perhaps no known implementation returns 0 buffers but that doesn't mean we can go and change the specification. Applications should expect and handle odp_packet_alloc_multi() returning 0 and not (by default) treat this as a fatal error. > >> >> + */ >>> +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int >>> num); >>> >> >> 'num' should be unsigned, a negative number doesn't make sense in this >> case. Although changing this breaks the ABI, so I'm not sure if we should >> fix this or just leave it. > > > Again this is copied directly from the other multi() APIs. Agree they > probably should be unsigned but I don't think it's a big deal since in the > unlikely event a negative number was supplied you wouldn't get any buffers > anyway. By the same argument specifying 0 here wouldn't make sense > semantically and yet that's a valid int and unsigned. > > >> >> >> ___ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > > > ___ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > > ___ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
