Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_queue_scalable.c
line 11
@@ -333,12 +333,12 @@ static odp_schedule_group_t queue_sched_group(odp_queue_t
handle)
return qentry_from_int(queue_from_ext(handle))->s.param.sched.group;
}
-static int queue_lock_count(odp_queue_t handle)
+static uint32_t queue_lock_count(odp_queue_t handle)
{
queue_entry_t *queue = qentry_from_int(queue_from_ext(handle));
return queue->s.param.sched.sync == ODP_SCHED_SYNC_ORDERED ?
- (int)queue->s.param.sched.lock_count : -1;
+ (int)queue->s.param.sched.lock_count : 0;
Comment:
Remove cast (int)
> Petri Savolainen(psavol) wrote:
> Cast to (int) should not be done anymore.
>> Balasubramanian Manoharan(bala-manoharan) wrote:
>> 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_r139078469
updated_at 2017-09-15 07:14:02
