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. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include "config.h" + +#include <odp_posix_extensions.h> +#include <odp/api/packet_io.h> +#include <odp/api/std_clib.h> +#include <odp/drv/shm.h> +#include "odp_packet_io_pool.h" +#include "odp_debug_internal.h" + +#define ODP_PKTIO_OPS_DATA_POOL_SIZE 160000
Comment: Where does this number come from? Is it subject to configuration? > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > 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_r151800200 updated_at 2017-11-20 12:52:30
