| Message ID | 1422982108-13813-6-git-send-email-ola.liljedahl@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 4 February 2015 at 10:51, 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 Ola Liljedahl >> Sent: Tuesday, February 03, 2015 6:48 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined behavior >> description >> >> Documented API calls which are guaranteed to handle invalid/stale handles. >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> --- >> (This document/code contribution attached is provided under the terms of >> agreement LES-LTM-21309) >> >> include/odp/api/buffer.h | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >> index 12b2f5a..1eb2e5a 100644 >> --- a/include/odp/api/buffer.h >> +++ b/include/odp/api/buffer.h >> @@ -88,7 +88,9 @@ uint32_t odp_buffer_size(odp_buffer_t buf); >> /** >> * Tests if buffer is valid >> * >> - * @param buf Buffer handle >> + * @param buf Buffer handle (possibly invalid) >> + * @note This is the only buffer API function which accepts invalid >> buffer >> + * handles (any bit value) without causing undefined behavior. >> * >> * @retval 1 Buffer handle represents a valid buffer. >> * @retval 0 Buffer handle does not represent a valid buffer. >> -- > > Originally, this and odp_packet_is_valid() were defined to enable full sanity check over the object. During debugging user could insert these on the way and (hopefully) catch the point where e.g. packet metadata (e.g. segment linking) was corrupted. Should we specify this "full sanity check of object" as well? That part is not obvious now and an ODP implementer could easily interpret "represents a valid buffer" in a less complete way. Perhaps this function needs more return values. E.g. @retval 0 Buffer handle is invalid (no further checks done) @retval 1 Buffer handle is valid but buffer is corrupted @retval 2 Buffer handle is valid and buffer seems correct > > So, I think this function could still crash on bad handles. Is there a use case that application would just want to check the handle validity (without crashing), but not the object sanity? I was thinking that some post-mortem dump code could iterate over all known (to the application) buffer and packet handles and save information about those. But in a crash situation, you can't really be sure that the handles you find are correct, e.g. could be stale handles so treated as invalid by an implementation. But you don't want to crash in your PMD code, in this case just save that this specific handle is invalid. > > -Petri > >
Handle validity and object consistency checking are two logically different operations. I think it makes more sense to separate these into two different API calls if they are needed. To date these APIs only perform handle validity checking and it's not clear what a "valid" object would be under the fuller interpretation. For example if I do an odp_packet_alloc() is the result valid? It's just space for a packet but obviously has no packet data. Or perhaps it has leftover bytes from a previous packet. Does that mean that garbage in a packet is 'valid' or that something I just allocated is somehow 'invalid'? On Wed, Feb 4, 2015 at 4:01 AM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 4 February 2015 at 10:51, 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 Ola Liljedahl > >> Sent: Tuesday, February 03, 2015 6:48 PM > >> To: lng-odp@lists.linaro.org > >> Subject: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined behavior > >> description > >> > >> Documented API calls which are guaranteed to handle invalid/stale > handles. > >> > >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > >> --- > >> (This document/code contribution attached is provided under the terms of > >> agreement LES-LTM-21309) > >> > >> include/odp/api/buffer.h | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h > >> index 12b2f5a..1eb2e5a 100644 > >> --- a/include/odp/api/buffer.h > >> +++ b/include/odp/api/buffer.h > >> @@ -88,7 +88,9 @@ uint32_t odp_buffer_size(odp_buffer_t buf); > >> /** > >> * Tests if buffer is valid > >> * > >> - * @param buf Buffer handle > >> + * @param buf Buffer handle (possibly invalid) > >> + * @note This is the only buffer API function which accepts invalid > >> buffer > >> + * handles (any bit value) without causing undefined behavior. > >> * > >> * @retval 1 Buffer handle represents a valid buffer. > >> * @retval 0 Buffer handle does not represent a valid buffer. > >> -- > > > > Originally, this and odp_packet_is_valid() were defined to enable full > sanity check over the object. During debugging user could insert these on > the way and (hopefully) catch the point where e.g. packet metadata (e.g. > segment linking) was corrupted. > Should we specify this "full sanity check of object" as well? That > part is not obvious now and an ODP implementer could easily interpret > "represents a valid buffer" in a less complete way. > Perhaps this function needs more return values. E.g. > @retval 0 Buffer handle is invalid (no further checks done) > @retval 1 Buffer handle is valid but buffer is corrupted > @retval 2 Buffer handle is valid and buffer seems correct > > > > > So, I think this function could still crash on bad handles. Is there a > use case that application would just want to check the handle validity > (without crashing), but not the object sanity? > I was thinking that some post-mortem dump code could iterate over all > known (to the application) buffer and packet handles and save > information about those. But in a crash situation, you can't really be > sure that the handles you find are correct, e.g. could be stale > handles so treated as invalid by an implementation. But you don't want > to crash in your PMD code, in this case just save that this specific > handle is invalid. > > > > > -Petri > > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 4 February 2015 at 13:28, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >> Sent: Wednesday, February 04, 2015 12:01 PM >> To: Savolainen, Petri (NSN - FI/Espoo) >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined >> behavior description >> >> On 4 February 2015 at 10:51, 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 Ola Liljedahl >> >> Sent: Tuesday, February 03, 2015 6:48 PM >> >> To: lng-odp@lists.linaro.org >> >> Subject: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined >> behavior >> >> description >> >> >> >> Documented API calls which are guaranteed to handle invalid/stale >> handles. >> >> >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> >> --- >> >> (This document/code contribution attached is provided under the terms >> of >> >> agreement LES-LTM-21309) >> >> >> >> include/odp/api/buffer.h | 4 +++- >> >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >> >> index 12b2f5a..1eb2e5a 100644 >> >> --- a/include/odp/api/buffer.h >> >> +++ b/include/odp/api/buffer.h >> >> @@ -88,7 +88,9 @@ uint32_t odp_buffer_size(odp_buffer_t buf); >> >> /** >> >> * Tests if buffer is valid >> >> * >> >> - * @param buf Buffer handle >> >> + * @param buf Buffer handle (possibly invalid) >> >> + * @note This is the only buffer API function which accepts invalid >> >> buffer >> >> + * handles (any bit value) without causing undefined behavior. >> >> * >> >> * @retval 1 Buffer handle represents a valid buffer. >> >> * @retval 0 Buffer handle does not represent a valid buffer. >> >> -- >> > >> > Originally, this and odp_packet_is_valid() were defined to enable full >> sanity check over the object. During debugging user could insert these on >> the way and (hopefully) catch the point where e.g. packet metadata (e.g. >> segment linking) was corrupted. >> Should we specify this "full sanity check of object" as well? That >> part is not obvious now and an ODP implementer could easily interpret >> "represents a valid buffer" in a less complete way. >> Perhaps this function needs more return values. E.g. >> @retval 0 Buffer handle is invalid (no further checks done) >> @retval 1 Buffer handle is valid but buffer is corrupted >> @retval 2 Buffer handle is valid and buffer seems correct > > Another option is to set errno to indicate the reason of failure. Valid vs. broken buffer/packet is the most valuable information from the call. It's secondary to know why it's broken. No need to change return values or add errno for v1.0. > > Also if this function needs to accept bad handles, I think it means that in general the handle structure/mapping to memory/etc have to be error tolerant, which we try to not force on other calls (by specifying undefined behavior on bad handles). To me it sounds all or nothing - either all buffer calls (that access only odp_buffer_t, no shared resources) have undefined behavior or non. > > So, I'd leave the patch out from the set, and improve documentation (add "sanity check") with another patch. Agreed. The problem is still not well understood. But the current function definition is weak and liable to be misunderstood. > >> >> > >> > So, I think this function could still crash on bad handles. Is there a >> use case that application would just want to check the handle validity >> (without crashing), but not the object sanity? >> I was thinking that some post-mortem dump code could iterate over all >> known (to the application) buffer and packet handles and save >> information about those. But in a crash situation, you can't really be >> sure that the handles you find are correct, e.g. could be stale >> handles so treated as invalid by an implementation. But you don't want >> to crash in your PMD code, in this case just save that this specific >> handle is invalid. > > Good point, but maybe post mortem tools are a separate topic (vendor specific). Can we standardize what happens / can be done after crash? I guess not. In my view, this post-mortem dumping is the responsibility of the application. ODP just needs to guarantee that I can (attempt to) inspect various ODP objects without crashing. Invalid handles, corrupt data structures etc should be reported back to the caller, not invoke undefined behavior. > > -Petri > >> >> > >> > -Petri >> > >> >
Was there anything you want to change in this patch? The only thing here is that we haven't really defined what "valid" means. /** * Tests if buffer is valid * * @param buf Buffer handle (possibly invalid) * @note This is the only buffer API function which accepts invalid buffer * handles (any bit value) without causing undefined behavior. * * @retval 1 Buffer handle represents a valid buffer. * @retval 0 Buffer handle does not represent a valid buffer. */ int odp_buffer_is_valid(odp_buffer_t buf); On 4 February 2015 at 13:42, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 4 February 2015 at 13:28, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: >> >> >>> -----Original Message----- >>> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >>> Sent: Wednesday, February 04, 2015 12:01 PM >>> To: Savolainen, Petri (NSN - FI/Espoo) >>> Cc: lng-odp@lists.linaro.org >>> Subject: Re: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined >>> behavior description >>> >>> On 4 February 2015 at 10:51, 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 Ola Liljedahl >>> >> Sent: Tuesday, February 03, 2015 6:48 PM >>> >> To: lng-odp@lists.linaro.org >>> >> Subject: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined >>> behavior >>> >> description >>> >> >>> >> Documented API calls which are guaranteed to handle invalid/stale >>> handles. >>> >> >>> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>> >> --- >>> >> (This document/code contribution attached is provided under the terms >>> of >>> >> agreement LES-LTM-21309) >>> >> >>> >> include/odp/api/buffer.h | 4 +++- >>> >> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >>> >> index 12b2f5a..1eb2e5a 100644 >>> >> --- a/include/odp/api/buffer.h >>> >> +++ b/include/odp/api/buffer.h >>> >> @@ -88,7 +88,9 @@ uint32_t odp_buffer_size(odp_buffer_t buf); >>> >> /** >>> >> * Tests if buffer is valid >>> >> * >>> >> - * @param buf Buffer handle >>> >> + * @param buf Buffer handle (possibly invalid) >>> >> + * @note This is the only buffer API function which accepts invalid >>> >> buffer >>> >> + * handles (any bit value) without causing undefined behavior. >>> >> * >>> >> * @retval 1 Buffer handle represents a valid buffer. >>> >> * @retval 0 Buffer handle does not represent a valid buffer. >>> >> -- >>> > >>> > Originally, this and odp_packet_is_valid() were defined to enable full >>> sanity check over the object. During debugging user could insert these on >>> the way and (hopefully) catch the point where e.g. packet metadata (e.g. >>> segment linking) was corrupted. >>> Should we specify this "full sanity check of object" as well? That >>> part is not obvious now and an ODP implementer could easily interpret >>> "represents a valid buffer" in a less complete way. >>> Perhaps this function needs more return values. E.g. >>> @retval 0 Buffer handle is invalid (no further checks done) >>> @retval 1 Buffer handle is valid but buffer is corrupted >>> @retval 2 Buffer handle is valid and buffer seems correct >> >> Another option is to set errno to indicate the reason of failure. Valid vs. broken buffer/packet is the most valuable information from the call. It's secondary to know why it's broken. No need to change return values or add errno for v1.0. >> >> Also if this function needs to accept bad handles, I think it means that in general the handle structure/mapping to memory/etc have to be error tolerant, which we try to not force on other calls (by specifying undefined behavior on bad handles). To me it sounds all or nothing - either all buffer calls (that access only odp_buffer_t, no shared resources) have undefined behavior or non. >> >> So, I'd leave the patch out from the set, and improve documentation (add "sanity check") with another patch. > Agreed. The problem is still not well understood. But the current > function definition is weak and liable to be misunderstood. > >> >>> >>> > >>> > So, I think this function could still crash on bad handles. Is there a >>> use case that application would just want to check the handle validity >>> (without crashing), but not the object sanity? >>> I was thinking that some post-mortem dump code could iterate over all >>> known (to the application) buffer and packet handles and save >>> information about those. But in a crash situation, you can't really be >>> sure that the handles you find are correct, e.g. could be stale >>> handles so treated as invalid by an implementation. But you don't want >>> to crash in your PMD code, in this case just save that this specific >>> handle is invalid. >> >> Good point, but maybe post mortem tools are a separate topic (vendor specific). Can we standardize what happens / can be done after crash? I guess not. > In my view, this post-mortem dumping is the responsibility of the > application. ODP just needs to guarantee that I can (attempt to) > inspect various ODP objects without crashing. Invalid handles, corrupt > data structures etc should be reported back to the caller, not invoke > undefined behavior. > > > >> >> -Petri >> >>> >>> > >>> > -Petri >>> > >>> >
diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h index 12b2f5a..1eb2e5a 100644 --- a/include/odp/api/buffer.h +++ b/include/odp/api/buffer.h @@ -88,7 +88,9 @@ uint32_t odp_buffer_size(odp_buffer_t buf); /** * Tests if buffer is valid * - * @param buf Buffer handle + * @param buf Buffer handle (possibly invalid) + * @note This is the only buffer API function which accepts invalid buffer + * handles (any bit value) without causing undefined behavior. * * @retval 1 Buffer handle represents a valid buffer. * @retval 0 Buffer handle does not represent a valid buffer.
Documented API calls which are guaranteed to handle invalid/stale handles. Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> --- (This document/code contribution attached is provided under the terms of agreement LES-LTM-21309) include/odp/api/buffer.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)