brbrooks replied on github web page:
include/odp/api/spec/pool.h
line 9
@@ -294,6 +294,22 @@ odp_pool_t odp_pool_lookup(const char *name);
typedef struct odp_pool_info_t {
const char *name; /**< pool name */
odp_pool_param_t params; /**< pool parameters */
+
+ /** Minimum data address.
+ * This is the minimum address that application accessible data of any
+ * object (event) allocated from the pool may locate. When there's no
+ * application accessible data (e.g. ODP_POOL_TIMEOUT pools), the
+ * value maybe zero.
Comment:
"maybe" -> "may be"
> sachin-saxena wrote
> Sure.
>> Petri Savolainen(psavol) wrote:
>> While adding commit log text, modify the style in these comments to begin
>> each row with a *.
>>
>> /**
>> *
>> *
>> */
>>> sachin-saxena wrote
>>> Looks much better. Thank you @psavol for your suggestions.
>>>> Petri Savolainen(psavol) wrote:
>>>> This spec should apply for all pool types (also other than packet). 0
>>>> address may be used when there's no data in an event (e.g. timeout).
>>>>
>>>> /** Minimum data address.
>>>> * This is the minimum address that application accessible data of any
>>>> * object (event) allocated from the pool may locate. When there's no
>>>> * application accessible data (e.g. ODP_POOL_TIMEOUT pools), the
>>>> * value maybe zero. */
>>>> uintptr_t min_data_addr;
>>>>
>>>> /** Maximum data address.
>>>> * This is the maximum address that application accessible data of any
>>>> * object (event) allocated from the pool may locate. When there's no
>>>> * application accessible data (e.g. ODP_POOL_TIMEOUT pools), the
>>>> * value maybe zero. */
>>>> uintptr_t max_data_addr;
>>>>> sachin-saxena wrote
>>>>> I will wait for other's comments, if any, before sending V3 patchset
>>>>>> sachin-saxena wrote
>>>>>> 1. Agreed for first suggestion.
>>>>>> 2. For Second, As per my understanding, the VPP only needs to know the
>>>>>> range of addresses in order to calculate OFFSET within. VPP doesn't
>>>>>> assume/store packet address beyond its lifetime (Rx to Tx)
>>>>>>> sachin-saxena wrote
>>>>>>> OK. agreed
>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>> The only other ambiguity is whether the address that applications see
>>>>>>>> for a packet is constant. VPP clearly assumes this but this is not
>>>>>>>> implied by the ODP spec. The "lifetime" of packet visibility is from
>>>>>>>> the time a call like `odp_packet_data()` is made until the thread
>>>>>>>> releases the `odp_packet_t` that was used to obtain this address. If
>>>>>>>> some other thread receives that handle and calls `odp_packet_data()`
>>>>>>>> there's no requirement or guarantee that it will get the same address
>>>>>>>> as the previous owner.
>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>> I'd change these to "visible address" rather than "address". The
>>>>>>>>> point is that the spec says nothing about how packets are represented
>>>>>>>>> within an implementation. It only states what applications may see by
>>>>>>>>> using other ODP APIs that manipulate odp_packet_t object.
>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>> I'd delete this note as it's not necessary and not complete for the
>>>>>>>>>> ODP API spec.
>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>> As we discussed earlier today. We should only need an extension to
>>>>>>>>>>> `odp_pool_info()` to return the `min_addr` and `max_addr` of any
>>>>>>>>>>> packet contained in the pool. This can simply be added to the end
>>>>>>>>>>> of the `odp_pool_info_t` struct. The application (in this case VPP)
>>>>>>>>>>> can then verify that the range is usable by it (_i.e.,_ is
>>>>>>>>>>> containable in 32 bits) and can store the info it needs to do its
>>>>>>>>>>> indexing from this info.
>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>> How sparse a "linear pool" may be? All implementations can claim
>>>>>>>>>>>> linear pool support by returning info.first_addr = 0,
>>>>>>>>>>>> info.last_addr = (uintptr_t) -1, where address size may be 64bits.
>>>>>>>>>>>>
>>>>>>>>>>>> Addresses could be used for debugging, but not much more than
>>>>>>>>>>>> that. What VPP is going to do with this information ?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>> @muvarov To your points:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. No, VPP only deals with packets so there's no need to
>>>>>>>>>>>>> generalize this since it is an accommodation for VPP, not
>>>>>>>>>>>>> something we want to encourage other applications to use.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2. We abandoned that approach because it required the application
>>>>>>>>>>>>> to know how much memory the ODP implementation needed for its
>>>>>>>>>>>>> internal use, which is not something it can reasonably know. So
>>>>>>>>>>>>> `odp_pool_create()` is responsible for allocating any shm used by
>>>>>>>>>>>>> the pool based on input provided by the caller.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3. Agree, the union seems strange here. Since having an output
>>>>>>>>>>>>> parameter in the `odp_pool_param_t` is not something we want this
>>>>>>>>>>>>> will have to be reworked anyway.
>>>>>>>>>>>>>> muvarov wrote
>>>>>>>>>>>>>> 1) is it needed for tmo and bufs also? why it's on only inside
>>>>>>>>>>>>>> packets? 2) How memory for *start_addr is allocated in VPP? In
>>>>>>>>>>>>>> previous version of odp_pool_create() was odp_shm_t parameter
>>>>>>>>>>>>>> which said to reuse already created shm. Maybe that needs to be
>>>>>>>>>>>>>> reconsidered. 3) why union is needed? why it's not with
>>>>>>>>>>>>>> uintptr_t ?
>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>> @Bill-Fischofer-Linaro yes, this looks like a good approach.
>>>>>>>>>>>>>>> @sachin-saxena could you please reimplement your PR following
>>>>>>>>>>>>>>> @Bill-Fischofer-Linaro 's suggestion?
>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>> Got confused between single-segment packet and linear space
>>>>>>>>>>>>>>>> for packets, sorry.
>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>> Yes the existing `seg_len` parameter is set to the minimum
>>>>>>>>>>>>>>>>> size segment that the application requires. This can be set
>>>>>>>>>>>>>>>>> to be equal to `max_len` to require single-segment packets.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> However @sachin-saxena mentioned at SFO17 that VPP can deal
>>>>>>>>>>>>>>>>> with multi-segment packets, though I'm not sure how that
>>>>>>>>>>>>>>>>> works.
>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>> VPP will not run on such platforms without modification. If
>>>>>>>>>>>>>>>>>> the goal is to not modify VPP then an
>>>>>>>>>>>>>>>>>> `odp_pool_capability()` output that says whether pools can
>>>>>>>>>>>>>>>>>> present a linear interface. During Connect @psavol also
>>>>>>>>>>>>>>>>>> mentioned that a separate API to get this info would be more
>>>>>>>>>>>>>>>>>> appropriate than bending the input to `odp_pool_create()` to
>>>>>>>>>>>>>>>>>> give output parameters.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> My personal preference would be something like:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> * `odp_pool_capability()` indicates whether pools can be
>>>>>>>>>>>>>>>>>> created that provide linear addressing.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> * An option on `odp_pool_create()` to request a linear pool.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> * A new API (or an extension to the existing
>>>>>>>>>>>>>>>>>> `odp_pool_info()` API) to get the mapping info that VPP
>>>>>>>>>>>>>>>>>> needs.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> That would seem to be more consistent with overall ODP
>>>>>>>>>>>>>>>>>> structure.
>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>> What will happen for platforms where `odp_packet_t` is not
>>>>>>>>>>>>>>>>>>> mapped to the virtual memory?
>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>> Isn't setting `seg_len` enough? Maybe we should provide an
>>>>>>>>>>>>>>>>>>>> updated definition for `seg_len` (e.g. in case `seg_len >
>>>>>>>>>>>>>>>>>>>> max_len`, always use single segment).
https://github.com/Linaro/odp/pull/200#discussion_r146438859
updated_at 2017-10-24 02:17:15
