bogdanPricope 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: Actual default size of this pool and the mechanism to automatically extend it will be part of another PR. This size covers requirements for two netmap interfaces (to pass netmap tests). > bogdanPricope wrote > I agree, this check should be part of odp_pktio_open() or > setup_pktio_entry() but this should be done in another PR. Here, I only move > this check upper in the function (is not a new code). >> nagarahalli wrote >> This file is for allocating the shmem for storing the driver's global data. >> >> The name 'pool' seems to be creating confusion with the packet/buffer pool. >> May be we should change the name of >> '_odp_ishm_pool_create/odpdrv_shm_pool_create'. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> This is a parameter passed through from `odp_pktio_open()`. The spec simply >>> says that this must be of type `ODP_POOL_PACKET`, so results are undefined >>> if the caller doesn't abide by the spec. A courtesy validation check >>> doesn't seem unreasonable, but any validation checking should be done in >>> the main API, not in each individual driver. As such drivers should assume >>> that `pool` is valid at entry since these entry points cannot be invoked >>> directly by applications. >>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>> `ODP_OPS_DATA_FREE()` >>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>> Again since this is a macro, the name should be `ODP_OPS_DATA_ALLOC()`. >>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>> Macros should be CAPITALIZED to indicate that they are macros rather >>>>>> than callable APIs. So `ODP_OPS_DATA()` would be used here. >>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>> 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_r151926565 updated_at 2017-11-20 12:52:30
