Re: [lng-odp] [PATCH API-NEXT v1] User pointer init to NULL - rebased
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp/api/plat/packet_types.h
line 6
@@ -67,101 +67,89 @@ typedef enum {
#endif
-/** @internal Packet field accessor */
+/** @cond _ODP_HIDE_FROM_DOXYGEN_ */
+
Comment:
OK
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> OK
>> Petri Savolainen(psavol) wrote:
>> This a section label for doxygen. We'll never add it to doxygen config or
>> define it otherwise. I think it just need to high light that this portion of
>> a file intentionally left hiden from doxygen documentation. "Internal" does
>> not tell that this is only a doxygen label, it would seem like a
>> configuration option or a preprocessor macro.
>>> Petri Savolainen(psavol) wrote:
>>> This makes the get side work consistently - code path and performance is
>>> the same for NULL values. The flag tells if NULL is returned, or pointer
>>> value needs to be read. Reading the pointer value may cause cache miss. The
>>> flag needs to be read in any case. If flag is zero, we can return NULL
>>> right away (no need to wait for cache to bring NULL value from memory).
>>>
>>> if (flags.user_ptr_set == 0)
>>> return NULL;
>>>
>>> All NULL cases avoid going to the memory:
>>> pkt = alloc()
>>> user_ptr(); <-- NULL
>>> user_ptr_set(pkt, );
>>> user_ptr(); <-- != NULL
>>> user_ptr_set(pkt, NULL);
>>> user_ptr(); <-- NULL
>>> user_ptr(); <-- NULL
>>> user_ptr(); <-- NULL
>>> user_ptr(); <-- NULL
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
This "optimization" is unnecessary. If the user wants to set the user ptr
to `NULL` that's as legitimate a value as any and will be retrieved
normally. All we care about is whether `odp_packet_user_ptr_set()` has
been called. The flag is reset by `odp_packet_reset()`.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> I like using this feature, but `_ODP_HIDE_FROM_DOXYGEN_` seems awkward
> here. Should we just use `_ODP_INTERNAL_` as a standard means of denoting
> internal items that should not appear in doxygen?
https://github.com/Linaro/odp/pull/392#discussion_r162240370
updated_at 2018-01-18 03:14:49
Re: [lng-odp] [PATCH API-NEXT v1] User pointer init to NULL - rebased
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 51
@@ -1259,7 +1260,15 @@ int odp_packet_input_index(odp_packet_t pkt)
void odp_packet_user_ptr_set(odp_packet_t pkt, const void *ptr)
{
- packet_hdr(pkt)->buf_hdr.user_ptr = ptr;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (odp_unlikely(ptr == NULL)) {
+ pkt_hdr->p.flags.user_ptr_set = 0;
+ return;
+ }
Comment:
OK
> Petri Savolainen(psavol) wrote:
> This a section label for doxygen. We'll never add it to doxygen config or
> define it otherwise. I think it just need to high light that this portion of
> a file intentionally left hiden from doxygen documentation. "Internal" does
> not tell that this is only a doxygen label, it would seem like a
> configuration option or a preprocessor macro.
>> Petri Savolainen(psavol) wrote:
>> This makes the get side work consistently - code path and performance is the
>> same for NULL values. The flag tells if NULL is returned, or pointer value
>> needs to be read. Reading the pointer value may cause cache miss. The flag
>> needs to be read in any case. If flag is zero, we can return NULL right away
>> (no need to wait for cache to bring NULL value from memory).
>>
>> if (flags.user_ptr_set == 0)
>> return NULL;
>>
>> All NULL cases avoid going to the memory:
>> pkt = alloc()
>> user_ptr(); <-- NULL
>> user_ptr_set(pkt, );
>> user_ptr(); <-- != NULL
>> user_ptr_set(pkt, NULL);
>> user_ptr(); <-- NULL
>> user_ptr(); <-- NULL
>> user_ptr(); <-- NULL
>> user_ptr(); <-- NULL
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> This "optimization" is unnecessary. If the user wants to set the user ptr
>>> to `NULL` that's as legitimate a value as any and will be retrieved
>>> normally. All we care about is whether `odp_packet_user_ptr_set()` has been
>>> called. The flag is reset by `odp_packet_reset()`.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
I like using this feature, but `_ODP_HIDE_FROM_DOXYGEN_` seems awkward
here. Should we just use `_ODP_INTERNAL_` as a standard means of denoting
internal items that should not appear in doxygen?
https://github.com/Linaro/odp/pull/392#discussion_r162240332
updated_at 2018-01-18 03:14:34
Re: [lng-odp] [PATCH API-NEXT v1] User pointer init to NULL - rebased
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/include/odp/api/plat/packet_types.h
line 6
@@ -67,101 +67,89 @@ typedef enum {
#endif
-/** @internal Packet field accessor */
+/** @cond _ODP_HIDE_FROM_DOXYGEN_ */
+
Comment:
This a section label for doxygen. We'll never add it to doxygen config or
define it otherwise. I think it just need to high light that this portion of a
file intentionally left hiden from doxygen documentation. "Internal" does not
tell that this is only a doxygen label, it would seem like a configuration
option or a preprocessor macro.
> Petri Savolainen(psavol) wrote:
> This makes the get side work consistently - code path and performance is the
> same for NULL values. The flag tells if NULL is returned, or pointer value
> needs to be read. Reading the pointer value may cause cache miss. The flag
> needs to be read in any case. If flag is zero, we can return NULL right away
> (no need to wait for cache to bring NULL value from memory).
>
> if (flags.user_ptr_set == 0)
> return NULL;
>
> All NULL cases avoid going to the memory:
> pkt = alloc()
> user_ptr(); <-- NULL
> user_ptr_set(pkt, );
> user_ptr(); <-- != NULL
> user_ptr_set(pkt, NULL);
> user_ptr(); <-- NULL
> user_ptr(); <-- NULL
> user_ptr(); <-- NULL
> user_ptr(); <-- NULL
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This "optimization" is unnecessary. If the user wants to set the user ptr to
>> `NULL` that's as legitimate a value as any and will be retrieved normally.
>> All we care about is whether `odp_packet_user_ptr_set()` has been called.
>> The flag is reset by `odp_packet_reset()`.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> I like using this feature, but `_ODP_HIDE_FROM_DOXYGEN_` seems awkward
>>> here. Should we just use `_ODP_INTERNAL_` as a standard means of denoting
>>> internal items that should not appear in doxygen?
https://github.com/Linaro/odp/pull/392#discussion_r161977196
updated_at 2018-01-17 07:58:24
Re: [lng-odp] [PATCH API-NEXT v1] User pointer init to NULL - rebased
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_packet.c
line 51
@@ -1259,7 +1260,15 @@ int odp_packet_input_index(odp_packet_t pkt)
void odp_packet_user_ptr_set(odp_packet_t pkt, const void *ptr)
{
- packet_hdr(pkt)->buf_hdr.user_ptr = ptr;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (odp_unlikely(ptr == NULL)) {
+ pkt_hdr->p.flags.user_ptr_set = 0;
+ return;
+ }
Comment:
This makes the get side work consistently - code path and performance is the
same for NULL values. The flag tells if NULL is returned, or pointer value
needs to be read. Reading the pointer value may cause cache miss. The flag
needs to be read in any case. If flag is zero, we can return NULL right away
(no need to wait for cache to bring NULL value from memory).
if (flags.user_ptr_set == 0)
return NULL;
All NULL cases avoid going to the memory:
pkt = alloc()
user_ptr(); <-- NULL
user_ptr_set(pkt, );
user_ptr(); <-- != NULL
user_ptr_set(pkt, NULL);
user_ptr(); <-- NULL
user_ptr(); <-- NULL
user_ptr(); <-- NULL
user_ptr(); <-- NULL
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> This "optimization" is unnecessary. If the user wants to set the user ptr to
> `NULL` that's as legitimate a value as any and will be retrieved normally.
> All we care about is whether `odp_packet_user_ptr_set()` has been called. The
> flag is reset by `odp_packet_reset()`.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> I like using this feature, but `_ODP_HIDE_FROM_DOXYGEN_` seems awkward here.
>> Should we just use `_ODP_INTERNAL_` as a standard means of denoting internal
>> items that should not appear in doxygen?
https://github.com/Linaro/odp/pull/392#discussion_r161976128
updated_at 2018-01-17 07:51:16
Re: [lng-odp] [PATCH API-NEXT v1] User pointer init to NULL - rebased
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp/api/plat/packet_types.h
line 6
@@ -67,101 +67,89 @@ typedef enum {
#endif
-/** @internal Packet field accessor */
+/** @cond _ODP_HIDE_FROM_DOXYGEN_ */
+
Comment:
I like using this feature, but `_ODP_HIDE_FROM_DOXYGEN_` seems awkward here.
Should we just use `_ODP_INTERNAL_` as a standard means of denoting internal
items that should not appear in doxygen?
https://github.com/Linaro/odp/pull/392#discussion_r161912461
updated_at 2018-01-16 23:05:12
Re: [lng-odp] [PATCH API-NEXT v1] User pointer init to NULL - rebased
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 51
@@ -1259,7 +1260,15 @@ int odp_packet_input_index(odp_packet_t pkt)
void odp_packet_user_ptr_set(odp_packet_t pkt, const void *ptr)
{
- packet_hdr(pkt)->buf_hdr.user_ptr = ptr;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (odp_unlikely(ptr == NULL)) {
+ pkt_hdr->p.flags.user_ptr_set = 0;
+ return;
+ }
Comment:
This "optimization" is unnecessary. If the user wants to set the user ptr to
`NULL` that's as legitimate a value as any and will be retrieved normally. All
we care about is whether `odp_packet_user_ptr_set()` has been called. The flag
is reset by `odp_packet_reset()`.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> I like using this feature, but `_ODP_HIDE_FROM_DOXYGEN_` seems awkward here.
> Should we just use `_ODP_INTERNAL_` as a standard means of denoting internal
> items that should not appear in doxygen?
https://github.com/Linaro/odp/pull/392#discussion_r161913072
updated_at 2018-01-16 23:05:12
