Balasubramanian Manoharan(bala-manoharan) replied on github web page:
include/odp/api/spec/schedule.h
line 43
@@ -365,7 +368,28 @@ void odp_schedule_order_lock(unsigned lock_index);
* hold this lock. Must be in the range
* 0..odp_queue_lock_count() - 1
*/
-void odp_schedule_order_unlock(unsigned lock_index);
+void odp_schedule_order_unlock(uint32_t lock_index);
+
+/**
+ * Release existing ordered context lock and acquire a new lock
+ *
+ * This call is valid only when holding an ordered synchronization context.
+ * Release a previously locked ordered context lock and acquire a new ordered
+ * context lock. The operation is equivalent to application calling first
+ * odp_schedule_order_unlock(unlock_index) and then
+ * odp_schedule_order_lock(lock_index). The same constraints apply to this call
+ * similar to calling these two calls in sequence.
+ *
+ * @param unlock_index Index of the acquired ordered lock in the current
+ * context to be released.
+ * @param lock_index Index of the ordered lock in the current context to be
+ * acquired. Must be in the range
+ * 0...odp_queue_lock_count() - 1.
+ *
+ * @see odp_schedule_order_lock(), odp_schedule_order_unlock()
+ *
+ */
+void odp_schedule_order_unlock_lock(uint32_t unlock_index, uint32_t
lock_index);
Comment:
changed in next verion.
> Balasubramanian Manoharan(bala-manoharan) wrote:
> Okay
>> Balasubramanian Manoharan(bala-manoharan) wrote:
>> Okay
>>> Petri Savolainen(psavol) wrote:
>>> Also lock_count should be updated to uint32_t.
>>>
>>> unsigned -> uint32_t
>>> typedef struct odp_schedule_param_t {
>>> ....
>>> unsigned lock_count;
>>> }
>>>
>>> int -> int32_t
>>> int odp_queue_lock_count(odp_queue_t queue);
>>>
>>> Check also validation code which uses int/unsigned variables today.
>>>> Petri Savolainen(psavol) wrote:
>>>> Also lock / unlock function pointer prototypes need to be updated to
>>>> uint32_t.
>>>>> Petri Savolainen(psavol) wrote:
>>>>> Maybe this "The same constraints apply to this call similar to calling
>>>>> these two calls in sequence" needs to be re-worded to be more fluent.
>>>>> E.g. "The same constraints apply with this call as with those two."
>>>>>> Petri Savolainen(psavol) wrote:
>>>>>> "Results are undefined if the
>>>>>> ordered lock with the given unlock_index is not held by
>>>>>> this ordered context."
>>>>>>
>>>>>> Duplication on unlock() API text. Better to remove this from here.
>>>>>>
>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>> It's better to remove this duplicate spec ...
>>>>>>> "This call is valid only when there is a single ordered context
>>>>>>> lock active. Results are undefined if this call is made when multiple
>>>>>>> ordered
>>>>>>> context locks are acquired in nested fashion.
>>>>>>> This call does not release the existing ordered context and only
>>>>>>> switches
>>>>>>> from one ordered lock to another ordered lock within the same ordered
>>>>>>> context."
>>>>>>>
>>>>>>> ... and refer to lock()/unlock() spec instead: "The operation is
>>>>>>> equivalent to application calling first
>>>>>>> odp_schedule_order_unlock(unlock_index) and then
>>>>>>> odp_schedule_order_lock(lock_index). The same constraints apply to this
>>>>>>> call than those two."
>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>> Remove this outdated sentence: " Previously acquired ordered lock is
>>>>>>>> released." We have now unlock_index to identify which lock is unlocked.
>>>>>>>>> muvarov wrote
>>>>>>>>> @psavol only comment like you did.
>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>> Was there some benefit of not having index of the unlocked lock as a
>>>>>>>>>> parameter ? Can't remember if I suggested it myself. Anyway,
>>>>>>>>>> implementation would simplify and it would still be as robust as
>>>>>>>>>> unlock() (which application may pass bad lock index).
>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>> Don't know how to comment on COMMIT LOG on Github !
>>>>>>>>>>>
>>>>>>>>>>> Anyway, commit log needs correct format "linux-gen: sched: ...."
>>>>>>>>>>> and some more text (it's empty now)
>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>> "This call does not release the existing ordered context and only
>>>>>>>>>>>> switches
>>>>>>>>>>>> from one ordered lock to another ordered lock within the same
>>>>>>>>>>>> ordered
>>>>>>>>>>>> context."
>>>>>>>>>>>>
>>>>>>>>>>>> I'd remove this since this call is not different to lock/unlock
>>>>>>>>>>>> calls in this regard
>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>> "This call is valid only when there is a single ordered context
>>>>>>>>>>>>> lock active. Results are undefined if this call is made when
>>>>>>>>>>>>> multiple ordered
>>>>>>>>>>>>> context locks are acquired in nested fashion. "
>>>>>>>>>>>>>
>>>>>>>>>>>>> Why nested locks are banned? I think it should not make a
>>>>>>>>>>>>> difference. Unlock_lock(C) is equivalent to unlock(B) + lock(C)
>>>>>>>>>>>>> under.
>>>>>>>>>>>>>
>>>>>>>>>>>>> lock(A)
>>>>>>>>>>>>> lock(B)
>>>>>>>>>>>>> unlock(B)
>>>>>>>>>>>>> lock(C)
>>>>>>>>>>>>> ...
>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>> To match other function prototypes, should be **unsigned**
>>>>>>>>>>>>>> instead of uint32_t.
>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>> It implements ordered lock API by handling all queues as atomic.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It demonstrates a very simple implementation of the API, and
>>>>>>>>>>>>>>> thus is an easy starting for new scheduler development.
>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>> I added this change for scenario where
>>>>>>>>>>>>>>>> odp_schedule_order_unlock_lock() is called after a lock has
>>>>>>>>>>>>>>>> been released using odp_schedule_order_unlock() but since we
>>>>>>>>>>>>>>>> are defining in the documentation this is an undefined
>>>>>>>>>>>>>>>> behaviour I believe your suggestion makes sense.
>>>>>>>>>>>>>>>> I will update in the next version.
>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>> Done
>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>> I will be moving lock_index from rctx to
>>>>>>>>>>>>>>>>>> sched_scalable_thread_state_t in the next version and thanks
>>>>>>>>>>>>>>>>>> for pointing it out.
>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>> The initial draft of ordered lock considered API for
>>>>>>>>>>>>>>>>>>> locking and unlocking ordered locks within a context. We
>>>>>>>>>>>>>>>>>>> found an use-case from the customer where the application
>>>>>>>>>>>>>>>>>>> was splitting a single ordered flow to two different flows,
>>>>>>>>>>>>>>>>>>> which is possible with our current API architecture but the
>>>>>>>>>>>>>>>>>>> drawback is that it requires the application to make two
>>>>>>>>>>>>>>>>>>> different calls odp_schedule_unlock() and
>>>>>>>>>>>>>>>>>>> odp_schedule_lock() which will have performance issues
>>>>>>>>>>>>>>>>>>> since the implementation has to internally switch out of
>>>>>>>>>>>>>>>>>>> the lock and then acquire the new lock. The main objective
>>>>>>>>>>>>>>>>>>> of this new API is to optimize this use-case by using a
>>>>>>>>>>>>>>>>>>> single API which switches the ordered context from one
>>>>>>>>>>>>>>>>>>> ordered lock to another.
>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>> Not true. Linux-generic implementation is using an
>>>>>>>>>>>>>>>>>>>> uint16_t but as explained before implementations are free
>>>>>>>>>>>>>>>>>>>> to use any number of bits as required. The number of bits
>>>>>>>>>>>>>>>>>>>> of lock_index also depends upon the application use-case.
>>>>>>>>>>>>>>>>>>>> Ordered locks is not related to number of threads in the
>>>>>>>>>>>>>>>>>>>> system but to the total number of flows in the system.
>>>>>>>>>>>>>>>>>>>> Depending upon the implementation it could support a huge
>>>>>>>>>>>>>>>>>>>> number of flows ( 2^20 ) and not all flows will be active.
>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>> Just to reiterate, ordered locks are used to make sure
>>>>>>>>>>>>>>>>>>>>> packet belonging to a particular flow acquire the lock in
>>>>>>>>>>>>>>>>>>>>> order. There is no need to acquire the lock in ascending
>>>>>>>>>>>>>>>>>>>>> order. Even though our linux-generic implementation is
>>>>>>>>>>>>>>>>>>>>> using them as index number for performance reasons it
>>>>>>>>>>>>>>>>>>>>> will usually be the last few bits of the flow id
>>>>>>>>>>>>>>>>>>>>> calculated by RSS or any other mechanism decided by the
>>>>>>>>>>>>>>>>>>>>> application.
>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>> As noted above, since `odp_schedule_order_unlock_lock()`
>>>>>>>>>>>>>>>>>>>>>> only takes one parameter (the index of the new lock to
>>>>>>>>>>>>>>>>>>>>>> be acquired), we need a place to store the index of the
>>>>>>>>>>>>>>>>>>>>>> lock that the caller now holds to be used as an implied
>>>>>>>>>>>>>>>>>>>>>> parameter to that API. So this is supposed to be a
>>>>>>>>>>>>>>>>>>>>>> thread-local variable. If that's not what's happening
>>>>>>>>>>>>>>>>>>>>>> here then that needs to be fixed, so thanks for pointing
>>>>>>>>>>>>>>>>>>>>>> that out.
>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>> The lock_index is an implied parameter to
>>>>>>>>>>>>>>>>>>>>>>> `odp_schedule_order_unlock_lock()`. It's the index of
>>>>>>>>>>>>>>>>>>>>>>> the lock that the caller currently holds, so this is a
>>>>>>>>>>>>>>>>>>>>>>> thread-local variable. It's an error to call this API
>>>>>>>>>>>>>>>>>>>>>>> without already holding an ordered lock.
>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> ODP does not have any architectural requirement that
>>>>>>>>>>>>>>>>>>>>>>>> ordered locks be acquired in any particular sequence,
>>>>>>>>>>>>>>>>>>>>>>>> or that they be acquired at all. The only requirement
>>>>>>>>>>>>>>>>>>>>>>>> is that they only can be acquired once per ordered
>>>>>>>>>>>>>>>>>>>>>>>> context. So this wording is correct. It's entirely
>>>>>>>>>>>>>>>>>>>>>>>> possible that a thread acquires lock 1 first and then
>>>>>>>>>>>>>>>>>>>>>>>> switches to lock 0. It's entirely up to the
>>>>>>>>>>>>>>>>>>>>>>>> application.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> The beauty of ordered locks is that they are
>>>>>>>>>>>>>>>>>>>>>>>> deadlock-free by design even in the face of random
>>>>>>>>>>>>>>>>>>>>>>>> acquisition order among the various threads that use
>>>>>>>>>>>>>>>>>>>>>>>> them.
>>>>>>>>>>>>>>>>>>>>>>>>> WonderfulVoid wrote
>>>>>>>>>>>>>>>>>>>>>>>>> @psavol The SP scheduler only exists to provide
>>>>>>>>>>>>>>>>>>>>>>>>> strict priority scheduling? The scalable scheduler
>>>>>>>>>>>>>>>>>>>>>>>>> does that as well (and doesn't the default scheduler
>>>>>>>>>>>>>>>>>>>>>>>>> also do basic SP scheduling?). Are there any specific
>>>>>>>>>>>>>>>>>>>>>>>>> characteristics of the SP scheduler (beyond the basic
>>>>>>>>>>>>>>>>>>>>>>>>> SP scheduling) that are of interest? In benchmarking,
>>>>>>>>>>>>>>>>>>>>>>>>> the SP scheduler seemed to be the slowest by far.
>>>>>>>>>>>>>>>>>>>>>>>>>> WonderfulVoid wrote
>>>>>>>>>>>>>>>>>>>>>>>>>> The reorder window (rwin) is shared by all threads,
>>>>>>>>>>>>>>>>>>>>>>>>>> you can't store per-thread data in it, you would use
>>>>>>>>>>>>>>>>>>>>>>>>>> the reorder context (rctx) for that.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> But why are we saving the lock_index? Is it used for
>>>>>>>>>>>>>>>>>>>>>>>>>> making any correctness checks (e.g. verifying that
>>>>>>>>>>>>>>>>>>>>>>>>>> the application isn't unlocking some lock it hasn't
>>>>>>>>>>>>>>>>>>>>>>>>>> acquired)?
>>>>>>>>>>>>>>>>>>>>>>>>>>> WonderfulVoid wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>> Why is this uint32_t? The lock_count (defined
>>>>>>>>>>>>>>>>>>>>>>>>>>> below) is only an uint16_t. We don't support more
>>>>>>>>>>>>>>>>>>>>>>>>>>> than 65535 locks (per ordered queue). The ordered
>>>>>>>>>>>>>>>>>>>>>>>>>>> locks themselves use uint32_t (for the "ticket")
>>>>>>>>>>>>>>>>>>>>>>>>>>> but also that is overkill, there just won't be that
>>>>>>>>>>>>>>>>>>>>>>>>>>> many threads.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Also this seems to be used for per-thread purposes
>>>>>>>>>>>>>>>>>>>>>>>>>>> (schedule_order_lock() saves the passed lock_index
>>>>>>>>>>>>>>>>>>>>>>>>>>> here) but the reorder_window is shared by all
>>>>>>>>>>>>>>>>>>>>>>>>>>> threads and cannot be used for per-thread data. If
>>>>>>>>>>>>>>>>>>>>>>>>>>> lock_index needs to be saved (is it actually used
>>>>>>>>>>>>>>>>>>>>>>>>>>> for anything?), it should be saved in the
>>>>>>>>>>>>>>>>>>>>>>>>>>> reorder_context.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> WonderfulVoid wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shouldn't lock_index be in the range
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1..odp_queue_lock_count()-1? We are supposedly
>>>>>>>>>>>>>>>>>>>>>>>>>>>> already holding one lock (lock_index >=0), this
>>>>>>>>>>>>>>>>>>>>>>>>>>>> lock will be released and the next lock will be
>>>>>>>>>>>>>>>>>>>>>>>>>>>> acquired.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Delete, as per previous explanation. The scalable
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> scheduler could be further optimized by changing
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> its runtime checks to ODP_ASSERT()s, but that's
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> beyond the scope of this PR.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This needs to go after the `if` statement, not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> inside it, since the whole statement is skipped
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> if the lock is immediately available--it's only
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> there to wait for the lock.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Delete, as per above explanation.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Again, this should have a doxygen comment for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> consistency.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Nice way to avoid the extraneous checking in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `schedule_order_unlock_lock()`, but actually
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this isn't needed at all due to the prior
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `ODP_ASSERT()`. That assert requires that the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `lock_index` lock be held at entry to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `schedule_order_unlock` so if
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_schedule_order_unlock_unlock()` is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> called without an ordered lock being held,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the previous `ODP_ASSERT()` will detect that
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and abort.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Interestingly, this also means that (in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> odp-linux, at least) you can call
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_schedule_order_unlock_lock()` while
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> holding nested ordered locks. The semantics
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is always to release the most recently
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> acquired ordered lock and replace that with a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> new ordered lock regardless of how many other
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ordered locks the caller holds in this
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ordered context.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> So the net is you can delete this statement
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> without any loss of correctness.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This should have a doxygen description for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> consistency with surrounding vars.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I didn't realize the schedule_sp didn't
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> implement ordered locks. @psavol is this a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> "to do" item?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> A simple reference to the stored
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `lock_index` is sufficient here. No need
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for atomics.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This can be optimized into an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `ODP_ASSERT()` to be consistent with the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> other schedulers. Behavior is undefined
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> if these routines are not called within
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> an ordered context, so no need to take a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> runtime hit to do these type of checks.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Same comment here. A simple `lock_index`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> tracker is all that's needed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Again, no need for fancy atomics here
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> since you're already protected by the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ordered lock itself. You're just
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> remembering the `lock_index` you locked.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I suspect same comments for this patch
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> apply equally to all of the scheduler
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> variants. Not repeating individual
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> review comments here.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Replace above two lines with:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> schedule_order_unlock(sched_local.ordered.lock_index);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Add here:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ODP_ASSERT(queue_index !=
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> NULL_INDEX &&
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lock_index <=
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sched->queue[queue_index].order_lock_count
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> &&
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> !sched_local.ordered.lock_called.u8[lock_index]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> &&
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sched_local.ordered.lock_called.u8[sched_local.ordered.lock_index]);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Unnecessary
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Unnecessary
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Unnecesssary
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> All that's needed here is a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `sched_local.ordered.lock_index
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> = lock_index;`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Not the best choice of name.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You're not tracking that a lock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> has been acquired, but rather
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the index value of the lock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that has been acquired.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Unnecessary.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Unnecessary
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Why does this need to be an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_atomic_u64_t`? All
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you're doing is tracking the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> index of the currently owned
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ordered lock so you can use
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this as an implicit
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> parameter to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_schedule_order_unlock_lock()`.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As such a `uint16_t` (or
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> even a `uint8_t`) should be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sufficient here.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> But this information is part
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of the `sched_local.ordered`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> struct.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You might wish to clarify
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that this call does not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> release the ordered context
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that contains the lock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> being unlocked. This call
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> simply changes from one
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ordered lock to another
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> within the current ordered
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> context.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Typo: acquire
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Typo: acquire
https://github.com/Linaro/odp/pull/160#discussion_r138858756
updated_at 2017-09-14 10:48:26
