Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page: include/odp/api/spec/packet_io.h @@ -1188,6 +1188,11 @@ uint64_t odp_pktin_ts_res(odp_pktio_t pktio); */ odp_time_t odp_pktin_ts_from_ns(odp_pktio_t pktio, uint64_t ns); +/** + * Packet IO operations data pool name + */ +#define ODP_PKTIO_OPS_DATA_POOL_NAME "ODP_PKTIO_OPS_DATA"
Comment: 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_r151796532 updated_at 2017-11-20 12:52:30
