Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/Makefile.am
line 4
@@ -175,6 +175,7 @@ noinst_HEADERS = \
include/odp_name_table_internal.h \
include/odp_packet_internal.h \
include/odp_packet_io_internal.h \
+ include/odp_packet_io_pool.h \
Comment:
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_r151797282
updated_at 2017-11-20 12:52:30
