Hi,
On 23/11/15 13:20, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----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.
We can swap those two instances of "buffer" to "packet" in a subsequent
patch.
This is packet API and uarea_init() is not an API function.
uarea_init() is a defined function pointer in the API, I think it's
quite clear what it is. Maybe a '*' character before it would be good to
emphasize that it's a function pointer, not a function?
Also, it should be well defined if area content is preserved,
"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."
always initialized (in alloc),
"It is not called from odp_packet_alloc(), unless the platform chooses
to allocate the memory at that point."
initialized only once (and not preserved)
"initialized exactly once with it when the underlying memory is allocated"
or not initialized.
"If the application specifies this pointer, it expects that every buffer
is initialized exactly once"
The spec is too loose now.
Your questions could be answered from the odp_packet_uarea_init_t
definition, let me know if there is a need for improvement in the wording.
*/
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
OVS only needs the first option, but we can extend this later on.
+ */
+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?
Do you mean what ODP packet metadata were initialized BEFORE this
callback were called? That's not defined, and couldn't be, as a lot of
these metadata could be undefined when the allocation of the packet
buffers happen, as the buffer is unused at that point by the packet. So
the packet length is not known, for example.
Looking through the API the following functions could be available at
the execution of the callback:
odp_packet_pool
odp_packet_to_event
Otherwise we should mention that the callback must not call other API
functions on the handle.
What metadata fields the callback can change?
As per above, nothing.
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.
OVS particularly needs to save the packet handle to the user area, hence
the handle passed.
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.
We can change that to "memory allocation time", but I think explaining
the whole purpose should be in the above description.
-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
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
