Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/pktio/tap.c
line 10
@@ -92,23 +92,30 @@ static int tap_pktio_open(odp_pktio_t id ODP_UNUSED,
int fd, skfd, flags;
uint32_t mtu;
struct ifreq ifr;
- pktio_ops_tap_data_t *tap = odp_ops_data(pktio_entry, tap);
+ pktio_ops_tap_data_t *tap = NULL;
if (strncmp(devname, "tap:", 4) != 0)
return -1;
+ if (pool == ODP_POOL_INVALID)
Comment:
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_r151808978
updated_at 2017-11-20 12:52:30
