Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_pktio_ops_subsystem.h
@@ -88,10 +89,40 @@ typedef ODP_MODULE_CLASS(pktio_ops) {
/* Maximum size of pktio specific ops data.*/
#define ODP_PKTIO_ODPS_DATA_MAX_SIZE 80000
+/* Pktio ops data pool */
+#define ODP_PKTIO_OPS_DATA_POOL_SIZE 160000
+#define ODP_PKTIO_OPS_DATA_POOL_NAME "ODP_PKTIO_OPS_DATA"
+
/* Extract pktio ops data from pktio entry structure */
#define odp_ops_data(_p, _mod) \
Comment:
Macros should be CAPITALIZED to indicate that they are macros rather than
callable APIs. So `ODP_OPS_DATA()` would be used here.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Where does this number come from? Is it subject to configuration?
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> 2017, not 2013 here.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 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_r151800809
updated_at 2017-11-20 12:52:30
