He Yi(heyi-linaro) replied on github web page:

platform/linux-generic/include/odp_packet_io_internal.h
line 6
@@ -43,8 +43,8 @@ typedef union pktio_entry_u pktio_entry_t;
 
 struct pktio_entry {
        const pktio_ops_module_t *ops;  /**< Implementation specific methods */
-       uint8_t ops_data[ODP_PKTIO_ODPS_DATA_MAX_SIZE]; /**< IO operation
-                                                       specific data */
+       void *ops_data;                 /**< IO operation specific data */


Comment:
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_r151591707
updated_at 2017-11-17 16:48:29

Reply via email to