> -----Original Message-----
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of EXT
> Maxim Uvarov
> Sent: Monday, November 23, 2015 2:41 PM
> To: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [API-NEXT PATCH v4 1/3] api: pool: add packet user
> area initializer for pool creation parameters
> 
> Merged,
> Maxim.


Hmmm. Didn't review this yet...


> 
> On 08/20/2015 09:45, Bala Manoharan wrote:
> > Reviewed-by: Balasubramanian Manoharan <bala.manoha...@linaro.org
> > <mailto:bala.manoha...@linaro.org>>
> >
> > On 15 August 2015 at 00:25, Zoltan Kiss <zoltan.k...@linaro.org
> > <mailto:zoltan.k...@linaro.org>> wrote:
> >
> >     Applications can preset certain parts of the packet user area, so
> >     when that
> >     memory will be allocated it starts from a known state. If the
> platform
> >     allocates the memory during pool creation, it's enough to run the
> >     constructor after that. If it's allocating memory on demand, it
> should
> >     call the constructor each time.
> >     Porting applications to ODP can benefit from this. If the
> >     application can't
> >     afford to change its whole packet handling to ODP, it's likely it
> >     needs to
> >     maintain its own metadata in the user area. And probably it needs
> >     to set
> >     constant fields in that metadata e.g. to mark that this is an ODP
> >     packet,
> >     and/or store the handle of the packet itself.
> >
> >     Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
> >     <mailto:zoltan.k...@linaro.org>>
> >     ---
> >     v2:
> >     - restrict this feature to packet user area
> >     - expand comments
> >
> >     v3:
> >     - include packet.h in pool.h
> >
> >     v4:
> >     - fix grammar based on Bill's comments
> >
> >      include/odp/api/packet.h |  3 +++
> >      include/odp/api/pool.h   | 26 ++++++++++++++++++++++++++
> >      2 files changed, 29 insertions(+)
> >
> >     diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h
> >     index 3a454b5..f5d2142 100644
> >     --- a/include/odp/api/packet.h
> >     +++ b/include/odp/api/packet.h
> >     @@ -73,6 +73,9 @@ extern "C" {
> >       * @note The default headroom and tailroom used for packets is
> >     specified by
> >       * the ODP_CONFIG_PACKET_HEADROOM and ODP_CONFIG_PACKET_TAILROOM
> >     defines in
> >       * odp_config.h.
> >     + * @note Data changed in user area might be preserved by the
> >     platform from
> >     + * previous usage of the buffer, so values preset in uarea_init()
> >     are not
> >     + * guaranteed.


Terms "buffer" and uarea_init() are ambiguous in the API spec. This is packet 
API and uarea_init() is not an API function.

Also, it should be well defined if area content is preserved, always 
initialized (in alloc), initialized only once (and not preserved) or not 
initialized. The spec is too loose now. 



> >       */
> >      odp_packet_t odp_packet_alloc(odp_pool_t pool, uint32_t len);
> >
> >     diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h
> >     index 2e79a55..01f770f 100644
> >     --- a/include/odp/api/pool.h
> >     +++ b/include/odp/api/pool.h
> >     @@ -21,6 +21,7 @@ extern "C" {
> >
> >
> >      #include <odp/std_types.h>
> >     +#include <odp/packet.h>
> >
> >      /** @defgroup odp_pool ODP POOL
> >       *  Operations on a pool.
> >     @@ -41,6 +42,23 @@ extern "C" {
> >      #define ODP_POOL_NAME_LEN  32
> >
> >      /**
> >     + * Packet user area initializer callback function for pools.
> >     + *
> >     + * @param pkt                   Handle of the packet
> >     + * @param uarea_init_arg        Opaque pointer defined in
> >     odp_pool_param_t
> >     + *
> >     + * @note If the application specifies this pointer, it expects
> >     that every buffer

Packet, not buffer


> >     + * is initialized exactly once with it when the underlying memory
> >     is allocated.
> >     + * It is not called from odp_packet_alloc(), unless the platform
> >     chooses to
> >     + * allocate the memory at that point. Applications can only
> >     assume that this
> >     + * callback is called once before the packet is first used. Any
> >     subsequent
> >     + * change to the user area might be preserved after
> >     odp_packet_free() is called,
> >     + * so applications should take care of (re)initialization if they
> >     change data
> >     + * preset by this function.


I think this should be two modes:
* init once and preserve
* init on every alloc



> >     + */
> >     +typedef void (odp_packet_uarea_init_t)(odp_packet_t pkt, void
> >     *uarea_init_arg);


Which packet fields have been correctly initialized when this callback is 
called? What metadata fields the callback can change?

If the intention is to init only uarea content, maybe it's better to just pass 
pointer and length (uarea size), instead of the entire packet.

typedef void (odp_packet_uarea_init_t)(void *user_area, uint32_t 
user_area_size, void *uarea_init_arg);



> >     +
> >     +/**
> >       * Pool parameters
> >       * Used to communicate pool creation options.
> >       */
> >     @@ -82,6 +100,14 @@ typedef struct odp_pool_param_t {
> >                             /** User area size in bytes. Specify as 0
> >     if no user
> >                                 area is needed. */
> >                             uint32_t uarea_size;
> >     +
> >     +                       /** Initialize every packet's user area at
> >     allocation
> >     +                           time. Use NULL if no initialization
> >     needed. */


"Allocation time" hints that it's called in every alloc.


-Petri



> >     +                       odp_packet_uarea_init_t *uarea_init;
> >     +
> >     +                       /** Opaque pointer passed to packet user
> area
> >     +                           constructor. */
> >     +                       void *uarea_init_arg;
> >                     } pkt;
> >                     struct {
> >                             /** Number of timeouts in the pool */
> >     --
> >     1.9.1
> >
> >     _______________________________________________
> >     lng-odp mailing list
> >     lng-odp@lists.linaro.org <mailto: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
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to