nagarahalli 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:
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_r151746667
updated_at 2017-11-17 17:52:49
