He Yi(heyi-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:
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_r151592945
updated_at 2017-11-17 16:48:29
