nagarahalli replied on github web page:
platform/linux-generic/Makefile.am
line 4
@@ -175,6 +175,7 @@ noinst_HEADERS = \
include/odp_name_table_internal.h \
include/odp_packet_internal.h \
include/odp_packet_io_internal.h \
+ include/odp_packet_io_pool.h \
Comment:
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_r151840292
updated_at 2017-11-20 12:52:30
