Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page: platform/linux-generic/odp_packet_io_pool.c @@ -0,0 +1,54 @@ +/* Copyright (c) 2013, Linaro Limited + * All rights reserved.
Comment: 2017, not 2013 here. > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > Are these intended to be new ODP APIs? If so they need to be specified in > `odp/api/spec/packet.h`. Or are they supposed to be new Southbound APIs for > use by drivers? Need some clarification here. The use of the `odp_` prefix > implies that these are Northbound APIs. If these are Southbound APIs, then > the prefix should be `odpdrv_`. >> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >> Both pools and shmem's are ODP objects. The difference is a pool is a >> structured collection of objects that can be allocated and freed from the >> pool and that contain both data and metadata, while a shmem is a "slab" of >> memory that has no structure beyond how the application chooses to use it. >> Given this distinction, a pool seems more useful here. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> 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_r151799781 updated_at 2017-11-20 12:52:30
