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