brbrooks replied on github web page:
include/odp/api/spec/pool.h
line 17
@@ -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.
+ */
+ 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.
Comment:
"maybe" -> "may be"
> brbrooks wrote
> "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_r146438879
updated_at 2017-10-24 02:17:15
