Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_packet_io_pool.h
@@ -0,0 +1,27 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP packet IO pool
+ */
+
+#ifndef ODP_PACKET_IO_POOL_H_
+#define ODP_PACKET_IO_POOL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int odp_packet_io_pool_create(void);
+int odp_packet_io_pool_destroy(void);
Comment:
Are these intended to be new ODP APIs? If so they need to be specified in
`odp/api/spec/packet.h`. Or are they supposed to be new Southbound APIs for use
by drivers? Need some clarification here. The use of the `odp_` prefix implies
that these are Northbound APIs. If these are Southbound APIs, then the prefix
should be `odpdrv_`.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Both pools and shmem's are ODP objects. The difference is a pool is a
> structured collection of objects that can be allocated and freed from the
> pool and that contain both data and metadata, while a shmem is a "slab" of
> memory that has no structure beyond how the application chooses to use it.
> Given this distinction, a pool seems more useful here.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This definitely should not be part of the API spec. It's an implementation
>> artifact.
>>> nagarahalli wrote
>>> This should be part of odp_pktio_ops_subsystem.h file.
>>>> nagarahalli wrote
>>>> Should we rename it to odp_packet_io_shm.h and odp_packet_io_shm.c?
>>>>> nagarahalli wrote
>>>>> '_p' is not required as the macro is returning 'ops_data'. Makes the
>>>>> macro simple as well.
>>>>>
>>>>> Similarly for odp_ops_data_free can just take 'ops_data' as input.
>>>>>
>>>>> This will be inline with future plans to not expose 'pktio_entry_t' to
>>>>> the drivers.
>>>>>> He Yi(heyi-linaro) wrote:
>>>>>> In future, since each pktio_ops module will not expose their private
>>>>>> data type, this macro can be changed to
>>>>>> >`#define odp_ops_data(_entry, _pdata) \
>>>>>> pdata = (typeof(_pdata))(_entry->s.ops_data)`
>>>>>>
>>>>>> So the odp_pktio_ops_subsystem.h header won't need to know all
>>>>>> pktio_ops' private data structure declaration. Can be considered next
>>>>>> time.
>>>>>>> He Yi(heyi-linaro) wrote:
>>>>>>> Like this!
>>>>>>>> He Yi(heyi-linaro) wrote:
>>>>>>>> Look good no more comments from me to this commit after Honnappa and
>>>>>>>> Josep's, this is a step forward for the pktio ops data
>>>>>>>>
>>>>>>>> This commit also reveals how complex in ODP to allocate an arbitrary
>>>>>>>> sized piece of memory, need to prepare a pool (and guess the largest
>>>>>>>> usage), lookup this pool in every allocation/free by name, and do the
>>>>>>>> allocation/free after then.
>>>>>>>>> He Yi(heyi-linaro) wrote:
>>>>>>>>> seems no need to add an extra macro here?
>>>>>>>>>
>>>>>>>>> ODP_SUBSYSTEM_FOREACH_TEMPLATE(...) is extern
>>>>>>>>> we can just use to generate a static function use the same macro:
>>>>>>>>> static ODP_SUBSYSTEM_FOREACH_TEMPLATE(...)
>>>>>>>>>> nagarahalli wrote
>>>>>>>>>> Temporarily, roundup the size to cache line size. This way all the
>>>>>>>>>> memory allocations will be cache line aligned.
>>>>>>>>>>> nagarahalli wrote
>>>>>>>>>>> Should be odp_ops_data_alloc(_p, size).
>>>>>>>>>>>
>>>>>>>>>>> As per the pkt I/O changes document, _p (pktio_entry_t) is not
>>>>>>>>>>> required to be exposed to drivers. Do you plan to do it as part of
>>>>>>>>>>> this PR?
>>>>>>>>>>>> nagarahalli wrote
>>>>>>>>>>>> Same here, this can be part of odp_pktio_term_global
>>>>>>>>>>>>> nagarahalli wrote
>>>>>>>>>>>>> This functionality can be done in odp_pktio_global_init function.
>>>>>>>>>>>>> This will avoid the changes to modular framework as well.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When we have done additional enhancements to shared memory, this
>>>>>>>>>>>>> code will be deleted. So, can be part of odp_pktio_global_init
>>>>>>>>>>>>> without affecting the modular framework.
>>>>>>>>>>>>>> Josep Puigdemont(joseppc) wrote:
>>>>>>>>>>>>>> ah, yes, true, I didn't think about this detail...
>>>>>>>>>>>>>>> bogdanPricope wrote
>>>>>>>>>>>>>>> @joseppc Btw, 'odp_ops_data_alloc(_p, _mod)' vs.
>>>>>>>>>>>>>>> odp_ops_data_alloc(_p, _size) ?
>>>>>>>>>>>>>>>> bogdanPricope wrote
>>>>>>>>>>>>>>>> No, this pool is used to allocate packets (for recv side).
>>>>>>>>>>>>>>>>> Josep Puigdemont(joseppc) wrote:
>>>>>>>>>>>>>>>>> Ok, I may be splitting hairs now, but I thought we were just
>>>>>>>>>>>>>>>>> checking whether the pool parameter passed to pktio_open was
>>>>>>>>>>>>>>>>> valid, and bail out if not. We are not actually using this
>>>>>>>>>>>>>>>>> pool to allocate this pktio's private data, right?
>>>>>>>>>>>>>>>>>> bogdanPricope wrote
>>>>>>>>>>>>>>>>>> Alloc/free vs. array has this disadvantage: you need to
>>>>>>>>>>>>>>>>>> allocate the memory at some point and free it if the
>>>>>>>>>>>>>>>>>> operation fails for any reason. It is better to delay the
>>>>>>>>>>>>>>>>>> allocation until after some common checks.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> stncmp: open calls are not on fast path ... no reason to
>>>>>>>>>>>>>>>>>> optimize the performance ... but repeated memory alloc/free
>>>>>>>>>>>>>>>>>> may affect some pool implementations
>>>>>>>>>>>>>>>>>>> bogdanPricope wrote
>>>>>>>>>>>>>>>>>>> It will require a cast when is called.
>>>>>>>>>>>>>>>>>>> A pktio_type may implement another way to allocate memory
>>>>>>>>>>>>>>>>>>> starting form the name of the pool / is not mandatory to
>>>>>>>>>>>>>>>>>>> use those macros but are helpful for existing pktio types.
>>>>>>>>>>>>>>>>>>>> bogdanPricope wrote
>>>>>>>>>>>>>>>>>>>> _p comes form (pktio_entry_t *)
>>>>>>>>>>>>>>>>>>>>> Josep Puigdemont(joseppc) wrote:
>>>>>>>>>>>>>>>>>>>>> It probably belongs to its own patch, but now that you
>>>>>>>>>>>>>>>>>>>>> are at it, it could even be moved even further up, as it
>>>>>>>>>>>>>>>>>>>>> is probably faster than checking for "tap:" in the device
>>>>>>>>>>>>>>>>>>>>> string.
>>>>>>>>>>>>>>>>>>>>>> Josep Puigdemont(joseppc) wrote:
>>>>>>>>>>>>>>>>>>>>>> (_p)? There are a couple more, also in
>>>>>>>>>>>>>>>>>>>>>> odp_ops_data_free.
>>>>>>>>>>>>>>>>>>>>>>> Josep Puigdemont(joseppc) wrote:
>>>>>>>>>>>>>>>>>>>>>>> Maybe we can return (void *)? This way we would not
>>>>>>>>>>>>>>>>>>>>>>> care if pktios name (or define) their private
>>>>>>>>>>>>>>>>>>>>>>> structures according to the naming conventions implicit
>>>>>>>>>>>>>>>>>>>>>>> in the macro.
https://github.com/Linaro/odp/pull/297#discussion_r151798925
updated_at 2017-11-20 12:52:30
