[PATCH 1/2] barrier: Remove leftover semaphore remnants

Martin Erik Werner martinerikwerner at gmail.com
Wed Oct 2 19:52:17 UTC 2019 

Thanks for the review! Discussion regarding suggested rewording and
cleanup are inline below.

One minor thing that I also noticed was in the background section there
is a "cticket" which looks like a typo in "Similarly, cticket holders
gather at the gates of arenas [...]". Unless it's some shorthand for
concert-tickets that I'm unaware of? Unless it's intended I'll add that
change to this patch set as well.

On Tue, 2019-10-01 at 18:02 -0500, Joel Sherrill wrote:
> 
> 
> On Tue, Oct 1, 2019, 5:07 PM Gedare Bloom <gedare at rtems.org> wrote:
> > On Tue, Oct 1, 2019 at 3:53 PM Gedare Bloom <gedare at rtems.org>
> > wrote:
> > >
> > > On Wed, Sep 4, 2019 at 3:36 PM Martin Erik Werner
> > > <martinerikwerner at gmail.com> wrote:
> > > >
> > > > Remove various incorrect references to "lock" and "obtain" and
> > to an
> > > > option set which is not part of the barrier interface.
> > > >
> > > > It looks like the barrier documentation was started based on a
> > copy of
> > > > the semaphore documentation and these things are surviving
> > remnants.
> > > >
> > > > Also remove an unfinished sentence in the barrier wait
> > description,
> > > > since the intended information is already provided in the under
> > the NOTE
> > > > label.
> > > Thanks for your contribution!  I have a few minor comments:
> > >
> > > > ---
> > > >  c-user/barrier_manager.rst | 38 +++++++-----------------------
> > --------
> > > >  1 file changed, 7 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/c-user/barrier_manager.rst b/c-
> > user/barrier_manager.rst
> > > > index b0bf3bf..e5d69b0 100644
> > > > --- a/c-user/barrier_manager.rst
> > > > +++ b/c-user/barrier_manager.rst
> > > > @@ -324,8 +324,7 @@ NOTES:
> > > >
> > > >  .. _rtems_barrier_wait:
> > > >
> > > > -.. index:: obtain a barrier
> > > > -.. index:: lock a barrier
> > > > +.. index:: wait at a barrier
> > > >  .. index:: rtems_barrier_wait
> > > >
> > > >  BARRIER_WAIT - Wait at a barrier
> > > > @@ -356,14 +355,11 @@ DIRECTIVE STATUS CODES:
> > > >
> > > >  DESCRIPTION:
> > > >
> > > > -    This directive acquires the barrier specified by ``id``. 
> > The
> > > > -    ``RTEMS_WAIT`` and ``RTEMS_NO_WAIT`` components of the
> > options parameter
> > > > -    indicate whether the calling task wants to wait for the
> > barrier to become
> > > > -    available or return immediately if the barrier is not
> > currently available.
> > > > -    With either ``RTEMS_WAIT`` or ``RTEMS_NO_WAIT``, if the
> > current barrier
> > > > -    count is positive, then it is decremented by one and the
> > barrier is
> > > > -    successfully acquired by returning immediately with a
> > successful return
> > > > -    code.
> > > > +    This directive waits at the barrier specified by ``id``. 
> > The timeout
> > > > +    parameter specifies the maximum interval the calling task
> > is willing to be
> > > > +    blocked waiting for the barrier.  If it is set to
> > ``RTEMS_NO_TIMEOUT``,
> > > > +    then the calling task will wait forever.  If the barrier
> > is available or
> > > 1. "will wait forever" -> "will wait forever if it doesn't get
> > > released" or something more precise, even just "may wait forever"
> 
> Will wait until the barrier is released/opened. Then add there is no
> option for no wait as found in other APIs. 

Ok, so
"If it is set to ``RTEMS_NO_TIMEOUT``, then the calling task will wait
until the barrier is released."
?

The whole "If the barrier is available or the ``RTEMS_NO_WAIT`` option
component is set, then the timeout is ignored." wording is another
semaphore leftover that I missed when moving things around.

I'm thinking I'll remove that whole sentence. replacing it with one of
the following alternatives (or a mix thereof):

    It is not possible to make the calling task return based on the timeout
    without wating for at least one clock tick.

    There is no equivalent to the ``RTEMS_NO_WAIT`` option for this directive,
    hence the minimum timeout possible is one clock tick.
   
    It is not possible to make the calling task return immediately based on the
    timeout, the minimum timeout is 1 clock tick.
   
    The minimum timeout interval is one clock tick and it is therefore not
    possible to make the calling task return immediately based on the timeout.

I'm still not particularly happy with any of them, but I'm a bit stuck
for the precise wording.

> > > 2. I'm not quite sure what "available" means in this context.
> > Maybe
> > > "open" is a better word?
> > >
> > I guess "available" is the wording that has been used throughout.
> > Again, probably due to inheriting from the semaphore documentation.
> > I
> > think "open/closed" is more precise when describing a barrier, but
> > available is OK if it is consistently applied.
> 
> Open/closed is better. I think of a barrier as the starting gates at
> a horse race. Horses arrive and wait while the gates are closed. When
> the race starts, all gates are opened and the horses are released. 
> > I would still like the fix on #1, even though that was not
> > introduced
> > by you just copied from elsewhere, it is good to improve it at the
> > same time.
> > 
> > > > +    the ``RTEMS_NO_WAIT`` option component is set, then
> > timeout is ignored.
> > > >
> > > >      Conceptually, the calling task should always be thought of
> > as blocking when
> > > >      it makes this call and being unblocked when the barrier is
> > released.  If
> > > > @@ -372,24 +368,7 @@ DESCRIPTION:
> > > >      callers will block except for the one which is the Nth
> > task which trips the
> > > >      automatic release condition.
> > > >
> > > > -    The timeout parameter specifies the maximum interval the
> > calling task is
> > > > -    willing to be blocked waiting for the barrier.  If it is
> > set to
> > > > -    ``RTEMS_NO_TIMEOUT``, then the calling task will wait
> > forever.  If the
> > > > -    barrier is available or the ``RTEMS_NO_WAIT`` option
> > component is set, then
> > > > -    timeout is ignored.
> > > > -
> > > >  NOTES:
> > > > -
> > > > -    The following barrier acquisition option constants are
> > defined by RTEMS:
> > > > -
> > > > -    .. list-table::
> > > > -     :class: rtems-table
> > > > -
> > > > -     * - ``RTEMS_WAIT``
> > > > -       - task will wait for barrier (default)
> > > > -     * - ``RTEMS_NO_WAIT``
> > > > -       - task should not wait
> > > > -
> > > >      A clock tick is required to support the timeout
> > functionality of this
> > > >      directive.
> > > >
> > > > @@ -399,7 +378,6 @@ NOTES:
> > > >
> > > >  .. _rtems_barrier_release:
> > > >
> > > > -.. index:: wait at a barrier
> > > >  .. index:: release a barrier
> > > >  .. index:: rtems_barrier_release
> > > >
> > > > @@ -425,9 +403,7 @@ DIRECTIVE STATUS CODES:
> > > >
> > > >  DESCRIPTION:
> > > >      This directive releases the barrier specified by id.  All
> > tasks waiting at
> > > > -    the barrier will be unblocked.  If the running task's
> > preemption mode is
> > > > -    enabled and one of the unblocked tasks has a higher
> > priority than the
> > > > -    running task.
> > > > +    the barrier will be unblocked.
> > > >
> > > >  NOTES:
> > > >      The calling task may be preempted if it causes a higher
> > priority task to be
> > > > --
> > > > 2.11.0
> > > >
> > > > _______________________________________________
> > > > devel mailing list
> > > > devel at rtems.org
> > > > http://lists.rtems.org/mailman/listinfo/devel
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
-- 
Martin Erik Werner <martinerikwerner at gmail.com>

More information about the devel mailing list