[RFC/API-NEXT] api: schedule: add ordered context unlock-lock

Message ID 1497612094-5991-1-git-send-email-bala.manoharan@linaro.org
State New
Headers show

Commit Message

Balasubramanian Manoharan June 16, 2017, 11:21 a.m.
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

Comments

Bill Fischofer June 16, 2017, 2:12 p.m. | #1
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

>
Honnappa Nagarahalli June 16, 2017, 3:44 p.m. | #2
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

>>
Balasubramanian Manoharan June 16, 2017, 5:12 p.m. | #3
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

> >

>
Balasubramanian Manoharan June 16, 2017, 5:13 p.m. | #4
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

> >>

>

Patch hide | download patch | download mbox

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);
+
+/**
  * @}
  */