On Fri, May 13, 2016 at 4:50 AM, Christophe Milard < [email protected]> wrote:
> > > On 12 May 2016 at 19:44, Bill Fischofer <[email protected]> wrote: > >> >> >> On Thu, May 12, 2016 at 8:24 AM, Christophe Milard < >> [email protected]> 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. > Yes, I noticed that too after I sent it. I'll remove it in v6. > >>> 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() > I don't think that is necessary. The arrows show relevant/permitted state transitions. Otherwise we'd have most other ODP APIs doing the same. You stay in TO_Delivered until one of the APIs shown on the arrows is called. > > >> >> >>> >>> 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. > I'll try to rework and expand on that a bit but the bottom line is I don't think that API is particularly well defined or useful. > >> >>> >>> 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 <[email protected]> >>> 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 < >>>> [email protected]> 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 <[email protected]> >>>>> 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 < >>>>>> [email protected]> 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 <[email protected]> >>>>>>> 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 < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 9 May 2016 at 16:28, Bill Fischofer <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Mon, May 9, 2016 at 7:58 AM, Christophe Milard < >>>>>>>>>> [email protected]> 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 < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Bill Fischofer <[email protected]> >>>>>>>>>>>> --- >>>>>>>>>>>> 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 >>>>>>>>>>>> [email protected] >>>>>>>>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
