bogdanPricope 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:
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_r151917124
updated_at 2017-11-20 12:52:30
