On Thu, May 12, 2016 at 8:24 AM, Christophe Milard <
christophe.mil...@linaro.org> wrote:
> Hi Bill,
> Just sent you a v4 containing my proposal regarding the FSMs and the text
> around it.
> I still have a few comments:
>
> First, your FSM mixed events and actions on the transition arrow. I could
> not get graphviz to have 2 colors on the same transition (one for events,
> one for actions), so I have restricted them to events. The only action
> worth mentioning is the event queuing action which is clearly described by
> your text (and a reminder in mine). The square node could be a way to
> represent it, but I don't like mixing event and action undistinguished. As
> long as it is clear and consistent (it seems I have a need for that these
> days...), I won't bother you.
>
I think it's fine this way. The expiration/schedule call adds detail
because a timeout isn't actually delivered until the scheduler returns it
to a thread, but I think this is clear enough here.
>
> Then a few questions:
>
> 1)My Time-out FSM does not show what happens to a time-out event when an
> odp_timer_free() is called while the timeout is in delivered state. I
> assume the time-out remains in delivered state until a
> odp_event_free/timeout_free() is called manually, but I am not sure (having
> 2 different FSM actally clarify this point). You are welcome to update my
> FSMs if we should keep them.
>
Nothing happens to the event since the association between timer and
timeout ends as soon as the timer expires. At that point the timer is in
the "Timer_Expired" state and the Timeout moves to the "Timeout_Delivered"
state (Timeout_Pending in my previous diagram). So they're no longer
coupled and operations on one no longer affect the other.
>
> 2)Your text says "Following start, applications may allocate, set, cancel,
> and free, timers...". I guess the comma after "timer" is a typo.
>
Yes, thanks for the correction.
>
> 3)The last sentence in your text is: "However, upon receiving a timeout
> event the application can use the odp_timeout_fresh() API to inquire
> whether the timeout event is fresh or stale". What is the definition of a
> stale timer?
>
This was a back reference to the earlier sentence discussing
odp_timer_cancel(): "An attempted cancel will fail if the timer is not set
or if it has already
expired." However, I recall this was an area that Ola and Petri debated
extensively when these were first defined. There is an implicit imprecision
in any efficient timer system since independent threads can be setting,
resetting, and cancelling timers while they are "in flight". Generally when
a timeout event is received it's the responsibility of the application to
determine whether that event is meaningful or not based on other
information it has (e.g., it's a TCP delayed ACK timer, but we've just
received a packet on that connection so I should ignore this event, etc.).
odp_timer_fresh() is a hook that allows the implementation to communicate
staleness information if it knows something that the application might not
otherwise know, but I expect most applications will just ignore this as a
reported "fresh" timer may still not be meaningful from the application's
perspective for reasons it knows.
>
> 4) And I kept the best for the end... :-)
> What about the validity of the pointer returned by odp_timeout_user_ptr()
> when the timer was set by odp thread A on a queue belonging to ODP thread B
> (B != A). Interresting question isn't it? :-).
>
>
The user ptr should really be revised for Tiger Moth to include a length
(just as we did for queues) so that it can be part of scheduler
prefetching. But as you know Monarch assumes everything is running in the
same address space. Make a note of this (and similar context pointers) as
this is something we need to cover in Tiger Moth process discussions.
> Thanks for reading...
>
After now reading this I don't think a v6 is needed unless you do.
>
> Christophe.
>
> On 12 May 2016 at 14:08, Bill Fischofer <bill.fischo...@linaro.org> wrote:
>
>> The green is for Timer related transitions while the blue is for Timeout
>> related transitions. This format also seemed to get rid of some of the
>> annoying line crossings. The red arrows are the transitions that relate to
>> timer expiration and resulting timeout event scheduling, neither of which
>> involve Timer APIs. So the colors were supposed to make things easier to
>> read.
>>
>> I'm all for improving clarity, so if you have a better way of diagraming
>> this that's great. I believe the diagrams and text go together in either
>> case and are intended to be taken together to improve understanding. The
>> goal of each of these sections of the User Guide should be to give readers
>> confidence that they understand how to use these APIs in writing their own
>> ODP applications, or for ODP implementers to understand the semantics they
>> need to provide in their own implementations of them.
>>
>> On Thu, May 12, 2016 at 3:13 AM, Christophe Milard <
>> christophe.mil...@linaro.org> wrote:
>>
>>> Hi,
>>>
>>> I am still confused with this shared FSM for the 2 distinct objects: The
>>> red seems to be actions associated with the transitions (and the timer
>>> expiration event is no longer shown on the transition showing the actions).
>>> I am not sure what the distinction between the green and blue events are.
>>> I propose the following: I will send a complete proposal to you,
>>> including both the FSMs and the text. If we still disagree on which one is
>>> clearer, we'll let other reviewer decide (you/I would sent a v4 with my
>>> stuff in). If no one does react, I am ok to mark your proposal as reviewed
>>> because some doc is better than none.
>>>
>>> So you are sure to win :-)
>>>
>>> Christophe.
>>>
>>> On 11 May 2016 at 01:47, Bill Fischofer <bill.fischo...@linaro.org>
>>> wrote:
>>>
>>>> I looked at them and I liked the added color so reworked my diagram
>>>> adding that and posted a v3 for it. I'm not sure trying to separate into
>>>> two diagrams really helps but perhaps this reworked diagram is easier to
>>>> follow?
>>>>
>>>> On Tue, May 10, 2016 at 3:45 AM, Christophe Milard <
>>>> christophe.mil...@linaro.org> wrote:
>>>>
>>>>> Hi Bill,
>>>>>
>>>>> Just sent you an RFC with 2 FSMs.
>>>>> Had to fight a bit with graphviz. Not finished yet. I will continue
>>>>> working on it if you think that makes sense...
>>>>> Christophe.
>>>>>
>>>>> On 9 May 2016 at 17:15, Bill Fischofer <bill.fischo...@linaro.org>
>>>>> wrote:
>>>>>
>>>>>> As we discussed in our call, we can both play around with graphviz
>>>>>> and see if a prettier diagram can be constructed that is perhaps clearer,
>>>>>> possibly with two separate FSMs.
>>>>>>
>>>>>> On Mon, May 9, 2016 at 9:39 AM, Christophe Milard <
>>>>>> christophe.mil...@linaro.org> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 9 May 2016 at 16:28, Bill Fischofer <bill.fischo...@linaro.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, May 9, 2016 at 7:58 AM, Christophe Milard <
>>>>>>>> christophe.mil...@linaro.org> wrote:
>>>>>>>>
>>>>>>>>> I am a bit confused by this diagram: It feels to me that timers
>>>>>>>>> and timeout have separate lives (even , if of course some dependency
>>>>>>>>> exist).
>>>>>>>>> From this diagram, it feels like these 2 separate objects (timers
>>>>>>>>> and time out events) share states...? do they?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Actually they do, which is what this diagram is trying to express.
>>>>>>>> When a timer is set one of the arguments is the timeout event that
>>>>>>>> should
>>>>>>>> be associated with it, so it is an error to attempt to free that event
>>>>>>>> while it is associated with a set timer (results are undefined if you
>>>>>>>> do).
>>>>>>>> Timers are somewhat unique in this respect.
>>>>>>>>
>>>>>>>
>>>>>>> Sorry, Bill I still don't get it: Aren't you saying here that these
>>>>>>> are 2 separate FSMs, but that these 2 separate FSMs are actually
>>>>>>> actionned/"triggered" by (partly) the same events? There is nothing
>>>>>>> wrong
>>>>>>> with that... Then they should be represented as 2 separated FSM with the
>>>>>>> same event names on the transitions triggered by the same events...
>>>>>>> Or I am very confused...
>>>>>>>
>>>>>>> Christophe.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> In my head, and from the understanding I had, it feels that the to
>>>>>>>>> 4 top states are for timers, whereas time-out events have only 2
>>>>>>>>> states:
>>>>>>>>> Unallocated or allocated.
>>>>>>>>> It feels you are doing 1 FSM out of two. Maybe , it should be two
>>>>>>>>> FSM (keeping the same transition names to show the dependancy.)
>>>>>>>>>
>>>>>>>>> And I guess there is a typo at "odp_timeout_freee().
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks. I'll fix that in the next version.
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Christophe
>>>>>>>>>
>>>>>>>>> On 7 May 2016 at 18:15, Bill Fischofer <bill.fischo...@linaro.org>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>> doc/images/.gitignore | 1 +
>>>>>>>>>> doc/images/timer_fsm.gv | 38
>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>>>>>> doc/users-guide/Makefile.am | 1 +
>>>>>>>>>> 3 files changed, 40 insertions(+)
>>>>>>>>>> create mode 100644 doc/images/timer_fsm.gv
>>>>>>>>>>
>>>>>>>>>> diff --git a/doc/images/.gitignore b/doc/images/.gitignore
>>>>>>>>>> index a19aa75..72cf7ec 100644
>>>>>>>>>> --- a/doc/images/.gitignore
>>>>>>>>>> +++ b/doc/images/.gitignore
>>>>>>>>>> @@ -1,2 +1,3 @@
>>>>>>>>>> resource_management.svg
>>>>>>>>>> pktio_fsm.svg
>>>>>>>>>> +timer_fsm.svg
>>>>>>>>>> diff --git a/doc/images/timer_fsm.gv b/doc/images/timer_fsm.gv
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 0000000..f8cb21a
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/doc/images/timer_fsm.gv
>>>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>>>> +digraph timer_state_machine {
>>>>>>>>>> + rankdir=LR;
>>>>>>>>>> + size="12,20";
>>>>>>>>>> + node [fontsize=28];
>>>>>>>>>> + edge [fontsize=28];
>>>>>>>>>> + node [shape=doublecircle]; Timer_Unalloc
>>>>>>>>>> + Timeout_Unalloc
>>>>>>>>>> + Timeout_Delivered;
>>>>>>>>>> + node [shape=rectangle]; Timeout_Queued;
>>>>>>>>>> + node [shape=circle];
>>>>>>>>>> + Timer_Unalloc -> Timer_Alloc [label="odp_timer_alloc()"];
>>>>>>>>>> + Timer_Alloc -> Timer_Unalloc [label="odp_timer_free()"];
>>>>>>>>>> + Timer_Alloc -> Timer_Set [label="odp_timer_set_abs()"];
>>>>>>>>>> + Timer_Alloc -> Timer_Set [label="odp_timer_set_rel()"];
>>>>>>>>>> + Timer_Set -> Timer_Alloc [label="odp_timer_cancel()"];
>>>>>>>>>> + Timer_Set -> Timeout_Alloc
>>>>>>>>>> + [label="odp_timer_cancel()"
>>>>>>>>>> constraint=false];
>>>>>>>>>> + Timer_Set -> Timeout_Queued [label="=>odp_queue_enq()"];
>>>>>>>>>> + Timeout_Queued -> Timeout_Delivered
>>>>>>>>>> [label="odp_schedule()"];
>>>>>>>>>> + Timeout_Unalloc -> Timeout_Alloc
>>>>>>>>>> + [label="odp_timeout_alloc()"
>>>>>>>>>> constraint=false];
>>>>>>>>>> + Timeout_Alloc -> Timeout_Unalloc
>>>>>>>>>> + [label="odp_timeout_free()"
>>>>>>>>>> constraint=false];
>>>>>>>>>> + Timeout_Alloc -> Timer_Set
>>>>>>>>>> + [label="odp_timer_set_abs()"
>>>>>>>>>> constraint=false];
>>>>>>>>>> + Timeout_Alloc -> Timer_Set
>>>>>>>>>> + [label="odp_timer_set_rel()"];
>>>>>>>>>> + Timeout_Delivered -> Timer_Unalloc
>>>>>>>>>> [label="odp_timer_free()"];
>>>>>>>>>> + Timeout_Delivered -> Timer_Set
>>>>>>>>>> [label="odp_timer_set_abs()"];
>>>>>>>>>> + Timeout_Delivered -> Timer_Set
>>>>>>>>>> [label="odp_timer_set_rel()"];
>>>>>>>>>> + Timeout_Delivered -> Timeout_Delivered
>>>>>>>>>> + [label="odp_timeout_from_event()"];
>>>>>>>>>> + Timeout_Delivered -> Timeout_Delivered
>>>>>>>>>> + [label="odp_timeout_timer()"];
>>>>>>>>>> + Timeout_Delivered -> Timeout_Unalloc
>>>>>>>>>> + [label="odp_event_free() /
>>>>>>>>>> odp_timeout_freee()"
>>>>>>>>>> + constraint=false];
>>>>>>>>>> +}
>>>>>>>>>> diff --git a/doc/users-guide/Makefile.am
>>>>>>>>>> b/doc/users-guide/Makefile.am
>>>>>>>>>> index 74caa96..6bb0131 100644
>>>>>>>>>> --- a/doc/users-guide/Makefile.am
>>>>>>>>>> +++ b/doc/users-guide/Makefile.am
>>>>>>>>>> @@ -30,6 +30,7 @@ IMAGES = $(top_srcdir)/doc/images/overview.svg \
>>>>>>>>>> $(top_srcdir)/doc/images/release_git.svg \
>>>>>>>>>> $(top_srcdir)/doc/images/segment.svg \
>>>>>>>>>> $(top_srcdir)/doc/images/simple_release_git.svg \
>>>>>>>>>> + $(top_srcdir)/doc/images/timer_fsm.svg \
>>>>>>>>>> $(top_srcdir)/doc/images/tm_hierarchy.svg \
>>>>>>>>>> $(top_srcdir)/doc/images/tm_node.svg
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> 2.5.0
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> lng-odp mailing list
>>>>>>>>>> lng-odp@lists.linaro.org
>>>>>>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
