Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_pktio_ops_subsystem.h
@@ -87,12 +87,8 @@ typedef ODP_MODULE_CLASS(pktio_ops) {
odp_api_proto(pktio_ops, print) print;
} pktio_ops_module_t;
-/* Maximum size of pktio specific ops data.*/
-#define ODP_PKTIO_ODPS_DATA_MAX_SIZE 80000
-
/* Extract pktio ops data from pktio entry structure */
-#define odp_ops_data(_p, _mod) \
- ((pktio_ops_ ## _mod ## _data_t *)(uintptr_t)_p->s.ops_data)
+#define odp_ops_data(_p, _mod) _p->s.ops_data
#define odp_ops_data_alloc(_p, _size) \
Comment:
Again since this is a macro, the name should be `ODP_OPS_DATA_ALLOC()`.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 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_r151800912
updated_at 2017-11-20 12:52:30
