On 12 May 2016 at 19:44, Bill Fischofer <bill.fischo...@linaro.org> wrote:
>
>
> 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.
>
>
On V5:
Maybe the error originated from me (?), but I see that the TO_Delivered
node is double ringed: Double ringed node would show the start state, so I
think it shouldn't be double ringed. If the intention was to show the final
state (though there isn't really one here: a TO event can be reused I
assume), then the Timer_Expired state should be double ringed as well. But
as mentionned, as I cannot clearly see any final state here, I would simply
remove the double ring from the TO_delivered state.
>
>> 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.
>
In that case there should be a transition TO_delivered->TO_delivered
labeled odp_timer_free()
>
>
>>
>> 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.
>
Not sure I understand that. I'll need explanations, and I guess the doc as
well.
>
>
>>
>> 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.
>
OK. Pity you are right here as I saw this as a good opportunity to trigger
some more opinions regarding the the process/thread debate... But you are
right: the aim is Monarch, and monarch just goes thread, so I cannot stop
you there :-)
I suggest we take a small HO as soon as possible (ping me when you want) so
you can explain the thing I don't understand and we can agree on a phrasing
around it on the fly.
Sorry, I see a need for V6, but I'll be on your side to make it the last
one :-)
Christophe.
>
>
>
>> 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
