| Message ID | 1497612094-5991-1-git-send-email-bala.manoharan@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 16, 2017 at 6:21 AM, Balasubramanian Manoharan <bala.manoharan@linaro.org> wrote: > Adds api to release an existing ordered lock and acquire a new lock. > > Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > --- > include/odp/api/spec/schedule.h | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/include/odp/api/spec/schedule.h b/include/odp/api/spec/schedule.h > index 8244746..c5d2214 100644 > --- a/include/odp/api/spec/schedule.h > +++ b/include/odp/api/spec/schedule.h > @@ -368,6 +368,26 @@ void odp_schedule_order_lock(unsigned lock_index); > void odp_schedule_order_unlock(unsigned lock_index); > > /** > + * Release ordered context lock and acquires new lock > + * > + * This call is valid only when holding an ordered synchronization context. > + * Release a previously locked ordered context lock and acquires Grammar: acquire > + * a new ordered context lock. > + * This call is valid only when there is a single ordered context lock and > + * is invalid if there are nested ordered context lock. Need to clarify what this means. I'm assuming the restriction is that the caller may only hold a single ordered lock at the time of the call? > + * > + * @param lock_index Index of the ordered lock in the current context to be > + * acquired. Previously acquired ordered lock is released. > + * Results are undefined if there is a nested ordered lock if the caller does not hold exactly one ordered lock? > + * in the context. 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 lock_index); Since odp_schedule_order_unlock() currently takes a lock index, it would be consistent (and eliminate a lot of the above confusion) if this were specified as: void odp_schedule_order_unlock_lock(uint32_t old_lock_index, uint32_t new_lock_index); That makes it clear which lock is being unlocked and which is being locked and means that it doesn't matter if one or more other locks are held at the time of this call. > + > +/** > * @} > */ > > -- > 1.7.9.5 >
Can you please explain the use case for this API? We already have odp_schedule_order_unlock and odp_schedule_order_lock. These APIs can be used to do the same operation. On 16 June 2017 at 09:12, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Fri, Jun 16, 2017 at 6:21 AM, Balasubramanian Manoharan > <bala.manoharan@linaro.org> wrote: >> Adds api to release an existing ordered lock and acquire a new lock. >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> --- >> include/odp/api/spec/schedule.h | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/include/odp/api/spec/schedule.h b/include/odp/api/spec/schedule.h >> index 8244746..c5d2214 100644 >> --- a/include/odp/api/spec/schedule.h >> +++ b/include/odp/api/spec/schedule.h >> @@ -368,6 +368,26 @@ void odp_schedule_order_lock(unsigned lock_index); >> void odp_schedule_order_unlock(unsigned lock_index); >> >> /** >> + * Release ordered context lock and acquires new lock >> + * >> + * This call is valid only when holding an ordered synchronization context. >> + * Release a previously locked ordered context lock and acquires > > Grammar: acquire > >> + * a new ordered context lock. >> + * This call is valid only when there is a single ordered context lock and >> + * is invalid if there are nested ordered context lock. > > Need to clarify what this means. I'm assuming the restriction is that > the caller may only hold a single ordered lock at the time of the > call? > >> + * >> + * @param lock_index Index of the ordered lock in the current context to be >> + * acquired. Previously acquired ordered lock is released. >> + * Results are undefined if there is a nested ordered lock > > if the caller does not hold exactly one ordered lock? > >> + * in the context. 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 lock_index); > > Since odp_schedule_order_unlock() currently takes a lock index, it > would be consistent (and eliminate a lot of the above confusion) if > this were specified as: > > void odp_schedule_order_unlock_lock(uint32_t old_lock_index, uint32_t > new_lock_index); > > That makes it clear which lock is being unlocked and which is being > locked and means that it doesn't matter if one or more other locks are > held at the time of this call. > >> + >> +/** >> * @} >> */ >> >> -- >> 1.7.9.5 >>
Regards, Bala On 16 June 2017 at 19:42, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Fri, Jun 16, 2017 at 6:21 AM, Balasubramanian Manoharan > <bala.manoharan@linaro.org> wrote: > > Adds api to release an existing ordered lock and acquire a new lock. > > > > Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > > --- > > include/odp/api/spec/schedule.h | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/include/odp/api/spec/schedule.h b/include/odp/api/spec/ > schedule.h > > index 8244746..c5d2214 100644 > > --- a/include/odp/api/spec/schedule.h > > +++ b/include/odp/api/spec/schedule.h > > @@ -368,6 +368,26 @@ void odp_schedule_order_lock(unsigned lock_index); > > void odp_schedule_order_unlock(unsigned lock_index); > > > > /** > > + * Release ordered context lock and acquires new lock > > + * > > + * This call is valid only when holding an ordered synchronization > context. > > + * Release a previously locked ordered context lock and acquires > > Grammar: acquire > Will change it. > > + * a new ordered context lock. > > + * This call is valid only when there is a single ordered context lock > and > > + * is invalid if there are nested ordered context lock. > > Need to clarify what this means. I'm assuming the restriction is that > the caller may only hold a single ordered lock at the time of the > call? > Yes. > > > + * > > + * @param lock_index Index of the ordered lock in the current context > to be > > + * acquired. Previously acquired ordered lock is > released. > > + * Results are undefined if there is a nested > ordered lock > > if the caller does not hold exactly one ordered lock? > Then this API should not be called in that scenario. > > > + * in the context. 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 lock_index); > > Since odp_schedule_order_unlock() currently takes a lock index, it > would be consistent (and eliminate a lot of the above confusion) if > this were specified as: > > void odp_schedule_order_unlock_lock(uint32_t old_lock_index, uint32_t > new_lock_index); > > That makes it clear which lock is being unlocked and which is being > locked and means that it doesn't matter if one or more other locks are > held at the time of this call. > The nested ordered context lock is not a common use-case from HW perspective, the common use-case is one where the user wants to move from one ordered ATOMIC flow to another ATOMIC flow and IMO by omitting the lock_index of the existing ordered lock it makes the API more user-friendly. If someone comes with an use-case of using this in a nested ordered lock then we may add a different API with the signature you have suggested. Regards, Bala > > > + > > +/** > > * @} > > */ > > > > -- > > 1.7.9.5 > > >
On 16 June 2017 at 21:14, Honnappa Nagarahalli < honnappa.nagarahalli@linaro.org> wrote: > Can you please explain the use case for this API? > > We already have odp_schedule_order_unlock and odp_schedule_order_lock. > These APIs can be used to do the same operation. > Yes. This API can be internally implemented using odp_schedule_order_unlock() followed by odp_schedule_order_lock() By combining these two APIs it makes configuration in the HW much easier. Regards, Bala > > On 16 June 2017 at 09:12, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > On Fri, Jun 16, 2017 at 6:21 AM, Balasubramanian Manoharan > > <bala.manoharan@linaro.org> wrote: > >> Adds api to release an existing ordered lock and acquire a new lock. > >> > >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > >> --- > >> include/odp/api/spec/schedule.h | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/include/odp/api/spec/schedule.h b/include/odp/api/spec/ > schedule.h > >> index 8244746..c5d2214 100644 > >> --- a/include/odp/api/spec/schedule.h > >> +++ b/include/odp/api/spec/schedule.h > >> @@ -368,6 +368,26 @@ void odp_schedule_order_lock(unsigned lock_index); > >> void odp_schedule_order_unlock(unsigned lock_index); > >> > >> /** > >> + * Release ordered context lock and acquires new lock > >> + * > >> + * This call is valid only when holding an ordered synchronization > context. > >> + * Release a previously locked ordered context lock and acquires > > > > Grammar: acquire > > > >> + * a new ordered context lock. > >> + * This call is valid only when there is a single ordered context lock > and > >> + * is invalid if there are nested ordered context lock. > > > > Need to clarify what this means. I'm assuming the restriction is that > > the caller may only hold a single ordered lock at the time of the > > call? > > > >> + * > >> + * @param lock_index Index of the ordered lock in the current > context to be > >> + * acquired. Previously acquired ordered lock is > released. > >> + * Results are undefined if there is a nested > ordered lock > > > > if the caller does not hold exactly one ordered lock? > > > >> + * in the context. 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 lock_index); > > > > Since odp_schedule_order_unlock() currently takes a lock index, it > > would be consistent (and eliminate a lot of the above confusion) if > > this were specified as: > > > > void odp_schedule_order_unlock_lock(uint32_t old_lock_index, uint32_t > > new_lock_index); > > > > That makes it clear which lock is being unlocked and which is being > > locked and means that it doesn't matter if one or more other locks are > > held at the time of this call. > > > >> + > >> +/** > >> * @} > >> */ > >> > >> -- > >> 1.7.9.5 > >> >
diff --git a/include/odp/api/spec/schedule.h b/include/odp/api/spec/schedule.h index 8244746..c5d2214 100644 --- a/include/odp/api/spec/schedule.h +++ b/include/odp/api/spec/schedule.h @@ -368,6 +368,26 @@ void odp_schedule_order_lock(unsigned lock_index); void odp_schedule_order_unlock(unsigned lock_index); /** + * Release ordered context lock and acquires new lock + * + * This call is valid only when holding an ordered synchronization context. + * Release a previously locked ordered context lock and acquires + * a new ordered context lock. + * This call is valid only when there is a single ordered context lock and + * is invalid if there are nested ordered context lock. + * + * @param lock_index Index of the ordered lock in the current context to be + * acquired. Previously acquired ordered lock is released. + * Results are undefined if there is a nested ordered lock + * in the context. 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 lock_index); + +/** * @} */
Adds api to release an existing ordered lock and acquire a new lock. Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> --- include/odp/api/spec/schedule.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) -- 1.7.9.5