On 30/11/15 13:31, Savolainen, Petri (Nokia - FI/Espoo) wrote:
diff --git a/include/odp/api/pool.h b/include/odp/api/pool.h
index 01f770f..7b258c3 100644
--- a/include/odp/api/pool.h
+++ b/include/odp/api/pool.h
@@ -42,21 +42,25 @@ extern "C" {
#define ODP_POOL_NAME_LEN 32
/**
- * Packet user area initializer callback function for pools.
+ * Packet user area initialize callback function
*
- * @param pkt Handle of the packet
- * @param uarea_init_arg Opaque pointer defined in
odp_pool_param_t
+ * Packet pool uses this callback function to initialize user area in
each
+ * packet. The function is called once (per packet) in pool creation
phase for
+ * both persistent (user_area.persistent is set) and non-persistent
user areas.
+ * Additionally, it is called to re-initialize non-persistent user
area, when
+ * ODP has overwritten the area content.
*
- * @note If the application specifies this pointer, it expects that
every 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.
+ * @param pool Packet pool handle
+ * @param init_arg User defined 'user_area.init_arg' pointer in
+ * odp_pool_param_t
+ * @param user_area Pointer to packet user area to initialize
+ * @param size User area size in bytes
+ * @param pkt_index Index of the packet. The range is from 0 to number
of
+ * packet in the pool (packet pool param 'num') minus
one.
Why would an application need this? It's not visible to the app. Also,
It's helpful for e.g. allocating per packet context memory at this point. This
is just unique, opaque index from 0 to num - 1.
my_packet_ctx_t my_packet_ctx[MAX_PACKETS];
user_area_init(odp_pool_t pool, void *init_arg, void *user_area, uint32_t size,
uint32_t pkt_index) {
my_packet_user_area = user_area;
my_packet_user_area->pkt_ctx = &my_packet_ctx[pkt_index];
}
And why couldn't you use the packet handle for that? This just seems to
me an another unique id for the packet.
This example seems to me like using the user area to store a pointer for
an application allocated piece of memory related for that particular
buffer. That would be a duplication of the functionality of
odp_packet_user_ptr()
ODP-DPDK for example allocates some extra number of rte_mbuf's because
the per-thread caching can sit on packets, which in extreme cases makes
impossible to allocate 'num' packets. See this for details:
https://git.linaro.org/lng/odp-
dpdk.git/commitdiff/e1ac6c797539a62ee6b93554a1f0a7f5ba433a36
So, what would be pkt_index for these extra buffers? (NB. 'num' is only
the number we MUST provide, nothing says we can't have more than that)
'num' is: "The number of packets that the pool must provide that are packet length 'len' bytes
or smaller" and "The number of packets may be less than 'num' when packets are larger
than 'len'"
So, it's actually the maximum number of packets that can be allocated from the
pool.
There could be less than 'num' packets if packets are bigger, yes, but
the description doesn't restrict us that we can't have more.
If you get the bill in the restaurant, you "must provide" that amount of
money, but nothing prevents you to pay more.
Any per thread caching causes variation to actual number in-flight, but any
time there should not be more than 'num' packets.
If you have a per-thread object cache for packets (sounds sensible to
avoid locking during allocation), after a short time you'll have an
arbitrary number of packets in those caches, waiting to be used. If the
app requests 'num' while all packets are free, you can't always fulfill
that request because some of the packets will be in other thread's
cache. There is an unit test to check that, that's when I introduced
this overcommit in odp-dpdk.
Of course that opens up the possibility that you can have more than
'num' packets allocated at one point, but I don't see it as a problem.
And as I said above, it isn't really forbidden in the spec.
If you say there is a good reason to make that restriction, we should
disable such object caches. (and change the API spec to be more explicit
on that restriction)
However, for OVS we need the odp_packet_t of the packet, so we can call
ODP functions on the packet. And if you pass it, everything except
init_arg becomes unnecessary, because you can just take them with the
ODP functions.
The packet handle and all packet metadata are (potentially) invalid at this
point.
My idea of this user area is that it's binded to the packet handle. This
is a key requirement from OVS
Packet handle and metadata may be created and filled with valid metadata only
after packet has created (by packet input HW or pool alloc in SW).
The handle could be even generation counted against double frees, etc SW/HW
bugs.
We should also add then that the init function should be called when the
handle is created. As mentioned above, from OVS point of view it is
important that this user area is bound to a handle.
The same area could be linked with different packet handles, descriptors, etc
during its life time.
*/
-typedef void (odp_packet_uarea_init_t)(odp_packet_t pkt, void
*uarea_init_arg);
+typedef void (odp_packet_user_area_init_t)(odp_pool_t pool, void
*init_arg,
+ void *user_area, uint32_t size,
+ uint32_t pkt_index);
/**
* Pool parameters
@@ -97,17 +101,35 @@ typedef struct odp_pool_param_t {
Use 0 for default. */
uint32_t seg_len;
- /** User area size in bytes. Specify as 0 if no user
- area is needed. */
- uint32_t uarea_size;
+ struct {
+ /** User area size in bytes. Specify as 0 if no
+ user area is needed. */
+ uint32_t size;
- /** Initialize every packet's user area at allocation
- time. Use NULL if no initialization needed. */
- odp_packet_uarea_init_t *uarea_init;
+ /** Callback function to initialize packet user
+ area. Use NULL if no initialization
+ needed. */
+ odp_packet_user_area_init_t *init;
- /** Opaque pointer passed to packet user area
- constructor. */
- void *uarea_init_arg;
+ /** Opaque pointer passed to packet user area
+ initialization callback function. */
+ void *init_arg;
+
+ /** 0: User area content may be overwritten when
+ packet is stored in the pool. If init
+ callback function is defined, it is
+ called to (re)initialize user area when
+ ever ODP has overwritten it. Init is not
+ necessarily called on every alloc, only
+ when needed (in pool creation and after
+ each overwrite).
+ 1: User area content must remain constant
+ when packet is stored in the pool. If
+ init callback function is defined, it is
+ called once in pool creation.
If the platform allocates fresh memory for every packet and release it
at odp_packet_free() (absolutely possible, we don't prevent it in any
way), how does it keep the content of the user area without memory?
Persistent user area may simplify application packet context management. If
user needs that, implementation must allocate user area from a memory location
that is not overwritten by the implementation.
Each packet needs to be updated with a pointer to an user area before returning
it to the user.
That happens already
Commonly this area would just extend a packet descriptor or packet buffer.
What is fresh memory in context of packet pools? Same handle (in different
times) may point to different packet buffer address?
Yes.
Handles are unique?
Yes, for me it is obvious that handles are uniquely identify an actual
packet in the ODP application. Even if they are randomly generated when
the platform takes the memory from the pool. Otherwise the platform
can't possibly determine e.g which packet's length is asked by the app.
And more importantly, what would be the benefit of such behavior? When
you allocate a packet from the pool, you get a random one, you won't
know if it was ever used before or not, if the user area was modified
since the first use or not etc.
Init callback initializes it. Application sees either initialized or already
used contexts. Application controls the state of the packet context (user area).
"0: User area content may be overwritten when packet is stored in the
pool. If init callback function is defined, it is called to
(re)initialize user area whenever ODP has changed the underlying memory,
or when the association between the packet handle and the user area changes.
Init is not necessarily called on every alloc, only when needed (in pool
creation, after each change by ODP in the underlying memory or the
associated handle)."
Instead of "overwritten" I think "changed the underlying memory" covers
better what we want to say. Also I've added the notion that changing the
association between the handle and the user area needs to provoke this
init function to be called.
"1: User area content must not be modified by the platform when packet
is stored in the pool. If init callback function is defined, it is
called once in pool creation."
As discussed on the call, this is the scenario when the app "caches"
resources used by packets (e.g. a timer) in the user area, so once they
are stored there at first use, the next time the user area is used with
a packet the application can reuse them. The implementation has to make
sure the memory stays there (even if normally it would allocate the
memory on the fly). I can see two problems with this:
- and what if the platform generates new handles for every new packet?
You may call the init function again (losing performance), but how would
the init function knows this is not pool creation time? Your pkt_index
idea would solve that, but I've mentioned above the other reasons why it
won't work.
- a more severe problem comes when you want to release those resources
stored in the user area. Allocating each packet and going through the
user area might be a _very_ dirty way to do that, but it still doesn't
work with the DPDK object cache scenario I've mentioned above.
What I need for OVS: to set up two fields in the user area once to store
data which will never change. OVS stores its packet structure there, one
field is to tell OVS this is an ODP packet, the other to store the
packet handle for a particular buffer. So I don't have to store these
data every time the packet is received from a pktio (== when it is
allocated). The real benefit comes when the platform allocate the user
area at pool creation time, so it has to do it only once, like probably
most platforms.
User area can hold constant parts of the OVS struct either it persistent or
non-persistent (fixed init func) user area.
Then on each packet, update packet handle and any other dynamic data into the
struct or pass those as arguments.
The packet handle IS half of the data we want to avoid initializing
every time we get the packet. The other is the source field, which is
the same value for every packet.
-Petri
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
