Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_pktio_ops_subsystem.h
@@ -92,6 +94,32 @@ typedef ODP_MODULE_CLASS(pktio_ops) {
#define odp_ops_data(_p, _mod) \
((pktio_ops_ ## _mod ## _data_t *)(uintptr_t)_p->s.ops_data)
+#define odp_ops_data_alloc(_p, _size) \
+({ \
+ odpdrv_shm_pool_t _pool; \
+ \
+ _p->s.ops_data = NULL; \
+ _pool = odpdrv_shm_pool_lookup(ODP_PKTIO_OPS_DATA_POOL_NAME); \
+ if (_pool != ODPDRV_SHM_POOL_INVALID) \
+ _p->s.ops_data = odpdrv_shm_pool_alloc(_pool, \
+ ROUNDUP_CACHE_LINE(_size)); \
+ \
+ _p->s.ops_data; \
+})
+
+#define odp_ops_data_free(_p) \
+({ \
Comment:
`ODP_OPS_DATA_FREE()`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 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_r151801694
updated_at 2017-11-20 12:52:30
