| Message ID | 1396061790-26705-1-git-send-email-santosh.shukla@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: > From: santosh shukla <santosh...@linaro.org <javascript:>> > > Signed-off-by: santosh shukla <santosh...@linaro.org <javascript:>> > --- > v2 change: > - added find and delete function so to delete only > tmo not the entire list. > > v3 change: > - moved tmo_buf free to application layer. > - changed serach and delete logic. > > platform/linux-generic/source/odp_timer.c | 63 > ++++++++++++++++++++++++++++- > 1 file changed, 61 insertions(+), 2 deletions(-) > > diff --git a/platform/linux-generic/source/odp_timer.c > b/platform/linux-generic/source/odp_timer.c > index 4bcc479..890dffb 100644 > --- a/platform/linux-generic/source/odp_timer.c > +++ b/platform/linux-generic/source/odp_timer.c > @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) > } > > > +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) > +{ > + timeout_t *cur, *prev; > + prev = NULL; > + > + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { > + if (cur->tmo_buf == handle) > + break; > + } > + > + if (prev == NULL) > + tmo = cur->next; If tmo is the first item in the list, the list breaks here. This does not remove cur from the list, but sets only the local variable tmo. > > + else > + prev->next = cur->next; > + > + if (!cur) { > + ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); > + return -1; > + } > + > + /* free tmo buf .. application to free */ > + > Timer is responsible of the tmo_buf (that itself allocated). So that needs to be frees. Not here though, since it's better to unlock first and let other cores access the tick list. Return tmo_buf from this function. Application is responsible of the (optional) tmo->buf that application allocated. -Petri
On 31 March 2014 02:22, Petri Savolainen <petri.savolainen@linaro.org> wrote: > > > On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: >> >> From: santosh shukla <santosh...@linaro.org> >> >> Signed-off-by: santosh shukla <santosh...@linaro.org> >> --- >> v2 change: >> - added find and delete function so to delete only >> tmo not the entire list. >> >> v3 change: >> - moved tmo_buf free to application layer. >> - changed serach and delete logic. >> >> platform/linux-generic/source/odp_timer.c | 63 >> ++++++++++++++++++++++++++++- >> 1 file changed, 61 insertions(+), 2 deletions(-) >> >> diff --git a/platform/linux-generic/source/odp_timer.c >> b/platform/linux-generic/source/odp_timer.c >> index 4bcc479..890dffb 100644 >> --- a/platform/linux-generic/source/odp_timer.c >> +++ b/platform/linux-generic/source/odp_timer.c >> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) >> } >> >> >> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >> +{ >> + timeout_t *cur, *prev; >> + prev = NULL; >> + >> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >> + if (cur->tmo_buf == handle) >> + break; >> + } >> + >> + if (prev == NULL) >> + tmo = cur->next; > > > If tmo is the first item in the list, the list breaks here. This does not > remove cur from the list, but sets only the local variable tmo. > I disagree, implementation will remove middle/last and first from list. if you have better solution then propose. > >> >> >> + else >> + prev->next = cur->next; >> + >> + if (!cur) { >> + ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); >> + return -1; >> + } >> + >> + /* free tmo buf .. application to free */ >> + > > > Timer is responsible of the tmo_buf (that itself allocated). So that needs > to be frees. Not here though, since it's better to unlock first and let > other cores access the tick list. Return tmo_buf from this function. > Why? tmo_buf very much accesible to application. .See ping_test application therefore it make sense to let application free tmo_buf. Also, whats use case for keeping two buffers in timeout_t { It confuses me,, when to allocate and free those buffers i.e.. buf and tmo_buf from timeout_t { > Application is responsible of the (optional) tmo->buf that application > allocated. > > -Petri > > > -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org. > For more options, visit https://groups.google.com/a/linaro.org/d/optout.
On 31 March 2014 12:16, Santosh Shukla <santosh.shukla@linaro.org> wrote: > On 31 March 2014 02:22, Petri Savolainen <petri.savolainen@linaro.org> wrote: >> >> >> On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: >>> >>> From: santosh shukla <santosh...@linaro.org> >>> >>> Signed-off-by: santosh shukla <santosh...@linaro.org> >>> --- >>> v2 change: >>> - added find and delete function so to delete only >>> tmo not the entire list. >>> >>> v3 change: >>> - moved tmo_buf free to application layer. >>> - changed serach and delete logic. >>> >>> platform/linux-generic/source/odp_timer.c | 63 >>> ++++++++++++++++++++++++++++- >>> 1 file changed, 61 insertions(+), 2 deletions(-) >>> >>> diff --git a/platform/linux-generic/source/odp_timer.c >>> b/platform/linux-generic/source/odp_timer.c >>> index 4bcc479..890dffb 100644 >>> --- a/platform/linux-generic/source/odp_timer.c >>> +++ b/platform/linux-generic/source/odp_timer.c >>> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) >>> } >>> >>> >>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >>> +{ >>> + timeout_t *cur, *prev; >>> + prev = NULL; >>> + >>> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >>> + if (cur->tmo_buf == handle) >>> + break; >>> + } >>> + >>> + if (prev == NULL) >>> + tmo = cur->next; >> >> >> If tmo is the first item in the list, the list breaks here. This does not >> remove cur from the list, but sets only the local variable tmo. >> > > I disagree, implementation will remove middle/last and first from > list. if you have better solution then propose. >> I see that whole condition should move inside for loop i.e.. logic for deleting head node in list or middle / last node. Other than that I find logic pretty elegant to address first entry in list type of condition. pasting a modified code snippet before sending v4 patch. With few other concern on freeing tmo_buf and buf of timeout_t struct.. mentioned in below snippet as a question. --------- /* * function to search and delete tmo entry from timeout list * as well free tmo_buif, buf.. * return 0 : on error.. handle not in list * 1 : success */ static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) { timeout_t *cur, *prev; prev = NULL; for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { if (cur->tmo_buf == handle) { if (prev == NULL) tmo = cur->next; else prev->next = cur->next; break; } } if (!cur) { ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); return 0; } /* free tmo buf .. application to free */ odp_buffer_free(cur->tmo_buf); /* ideally delete operation should free tmo_buf, buf then list entry i.e.. cur though not sure appropriate api to delete cur entry from list, also conditional use-case for freeing buf of timeout_t..???*/ /*odp_buffer_free(cur->buf); odp_buffer_free(cur);*/ return 1; } ----------- >>> >>> >>> + else >>> + prev->next = cur->next; >>> + >>> + if (!cur) { >>> + ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); >>> + return -1; >>> + } >>> + >>> + /* free tmo buf .. application to free */ >>> + >> >> >> Timer is responsible of the tmo_buf (that itself allocated). So that needs >> to be frees. Not here though, since it's better to unlock first and let >> other cores access the tick list. Return tmo_buf from this function. >> > > Why? tmo_buf very much accesible to application. .See ping_test > application therefore it make sense to let application free tmo_buf. > > Also, whats use case for keeping two buffers in timeout_t { It > confuses me,, when to allocate and free those buffers i.e.. buf and > tmo_buf from timeout_t { > >> Application is responsible of the (optional) tmo->buf that application >> allocated. >> >> -Petri >> >> >> -- >> You received this message because you are subscribed to the Google Groups >> "LNG ODP Sub-team - lng-odp@linaro.org" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to lng-odp+unsubscribe@linaro.org. >> To post to this group, send email to lng-odp@linaro.org. >> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. >> To view this discussion on the web visit >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org. >> For more options, visit https://groups.google.com/a/linaro.org/d/optout.
Two points. This timer API that was sneaked into ODP is flawed. E.g. you can get different errors (although it might not happen with this specific implementation) when arming/resetting/disarming timers which is done when a flow is already set up and you really really don't want to fail resetting a timer because that will cause you to tear down the flow (if you can't supervise it, you can't keep it or you might leak resources and be subject to DoS situations) and add a lot of complexity to the application. This implementation is also crap, we can't traverse over linked lists when looking for timers. How will that work with more than say 10 active timers? Even linux-generic is worthy a better implementation. Which I actually have and now is able to contribute. A timer implementation using a priority queue based on a 4-ary heap (all children of each node fit into the same 64-byte cache line). Seems to scale well to hundreds of thousand of timers, only a couple of cache lines accessed for each timer reset operation. So I suggest we don't waste more time on this current implementation. On 1 April 2014 11:39, Santosh Shukla <santosh.shukla@linaro.org> wrote: > On 31 March 2014 12:16, Santosh Shukla <santosh.shukla@linaro.org> wrote: > > On 31 March 2014 02:22, Petri Savolainen <petri.savolainen@linaro.org> > wrote: > >> > >> > >> On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: > >>> > >>> From: santosh shukla <santosh...@linaro.org> > >>> > >>> Signed-off-by: santosh shukla <santosh...@linaro.org> > >>> --- > >>> v2 change: > >>> - added find and delete function so to delete only > >>> tmo not the entire list. > >>> > >>> v3 change: > >>> - moved tmo_buf free to application layer. > >>> - changed serach and delete logic. > >>> > >>> platform/linux-generic/source/odp_timer.c | 63 > >>> ++++++++++++++++++++++++++++- > >>> 1 file changed, 61 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/platform/linux-generic/source/odp_timer.c > >>> b/platform/linux-generic/source/odp_timer.c > >>> index 4bcc479..890dffb 100644 > >>> --- a/platform/linux-generic/source/odp_timer.c > >>> +++ b/platform/linux-generic/source/odp_timer.c > >>> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) > >>> } > >>> > >>> > >>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) > >>> +{ > >>> + timeout_t *cur, *prev; > >>> + prev = NULL; > >>> + > >>> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { > >>> + if (cur->tmo_buf == handle) > >>> + break; > >>> + } > >>> + > >>> + if (prev == NULL) > >>> + tmo = cur->next; > >> > >> > >> If tmo is the first item in the list, the list breaks here. This does > not > >> remove cur from the list, but sets only the local variable tmo. > >> > > > > I disagree, implementation will remove middle/last and first from > > list. if you have better solution then propose. > >> > > I see that whole condition should move inside for loop i.e.. logic for > deleting head node in list or middle / last node. Other than that I > find logic pretty elegant to address first entry in list type of > condition. pasting a modified code snippet before sending v4 patch. > > With few other concern on freeing tmo_buf and buf of timeout_t > struct.. mentioned in below snippet as a question. > > --------- > /* > * function to search and delete tmo entry from timeout list > * as well free tmo_buif, buf.. > * return 0 : on error.. handle not in list > * 1 : success > */ > > static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) > { > timeout_t *cur, *prev; > prev = NULL; > > for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { > if (cur->tmo_buf == handle) { > if (prev == NULL) > tmo = cur->next; > else > prev->next = cur->next; > > break; > } > } > > if (!cur) { > ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); > return 0; > } > > /* free tmo buf .. application to free */ > odp_buffer_free(cur->tmo_buf); > > /* ideally delete operation should free tmo_buf, buf then list > entry i.e.. cur > though not sure appropriate api to delete cur entry from > list, also conditional use-case > for freeing buf of timeout_t..???*/ > /*odp_buffer_free(cur->buf); > > odp_buffer_free(cur);*/ > return 1; > } > ----------- > > > >>> > >>> > >>> + else > >>> + prev->next = cur->next; > >>> + > >>> + if (!cur) { > >>> + ODP_ERR("Couldn't find the tmo handle (%d)\n", > handle); > >>> + return -1; > >>> + } > >>> + > >>> + /* free tmo buf .. application to free */ > >>> + > >> > >> > >> Timer is responsible of the tmo_buf (that itself allocated). So that > needs > >> to be frees. Not here though, since it's better to unlock first and let > >> other cores access the tick list. Return tmo_buf from this function. > >> > > > > Why? tmo_buf very much accesible to application. .See ping_test > > application therefore it make sense to let application free tmo_buf. > > > > Also, whats use case for keeping two buffers in timeout_t { It > > confuses me,, when to allocate and free those buffers i.e.. buf and > > tmo_buf from timeout_t { > > > >> Application is responsible of the (optional) tmo->buf that application > >> allocated. > >> > >> -Petri > >> > >> > >> -- > >> You received this message because you are subscribed to the Google > Groups > >> "LNG ODP Sub-team - lng-odp@linaro.org" group. > >> To unsubscribe from this group and stop receiving emails from it, send > an > >> email to lng-odp+unsubscribe@linaro.org. > >> To post to this group, send email to lng-odp@linaro.org. > >> Visit this group at > http://groups.google.com/a/linaro.org/group/lng-odp/. > >> To view this discussion on the web visit > >> > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org > . > >> For more options, visit https://groups.google.com/a/linaro.org/d/optout > . > > -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CA%2BiXiiNL3wNz40JkPktV8k%3DXhd30vsPMY07%2BbgVqz%3DWZPKoD9Q%40mail.gmail.com > . > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
Hi, I have told you this already - it's the first draft, everybody knows that it's not complete. I put it there to kick start development and expected Santosh, and maybe even you, to continue from that. Too near / too far / etc errors / feature xyz needs to be added - it's not a big deal. If you feel passionate - contribute code. And again, it's not complete implementation. It's crap - everybody knows that. But it's more that word on mailing list - it actually delivers timeouts today. Again, expected other guys to pick it up. Of course list search per cancel is crab. But again, what if the list would a double-linked list. You have a handle to the tmo and can remove it from the list with touching only three cache lines, which may be just good enough (or even better than a more complicated structure). Note locking is per tmo list and there are many tmos, average lock contention is low. -Petri On Tuesday, 1 April 2014 16:55:19 UTC+3, Ola Liljedahl wrote: > > Two points. > > This timer API that was sneaked into ODP is flawed. E.g. you can get > different errors (although it might not happen with this specific > implementation) when arming/resetting/disarming timers which is done when a > flow is already set up and you really really don't want to fail resetting a > timer because that will cause you to tear down the flow (if you can't > supervise it, you can't keep it or you might leak resources and be subject > to DoS situations) and add a lot of complexity to the application. > > This implementation is also crap, we can't traverse over linked lists when > looking for timers. How will that work with more than say 10 active timers? > Even linux-generic is worthy a better implementation. Which I actually have > and now is able to contribute. A timer implementation using a priority > queue based on a 4-ary heap (all children of each node fit into the same > 64-byte cache line). Seems to scale well to hundreds of thousand of timers, > only a couple of cache lines accessed for each timer reset operation. > > So I suggest we don't waste more time on this current implementation. > > > > On 1 April 2014 11:39, Santosh Shukla <santosh...@linaro.org <javascript:> > > wrote: > >> On 31 March 2014 12:16, Santosh Shukla <santosh...@linaro.org<javascript:>> >> wrote: >> > On 31 March 2014 02:22, Petri Savolainen <petri.sa...@linaro.org<javascript:>> >> wrote: >> >> >> >> >> >> On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: >> >>> >> >>> From: santosh shukla <santosh...@linaro.org> >> >>> >> >>> Signed-off-by: santosh shukla <santosh...@linaro.org> >> >>> --- >> >>> v2 change: >> >>> - added find and delete function so to delete only >> >>> tmo not the entire list. >> >>> >> >>> v3 change: >> >>> - moved tmo_buf free to application layer. >> >>> - changed serach and delete logic. >> >>> >> >>> platform/linux-generic/source/odp_timer.c | 63 >> >>> ++++++++++++++++++++++++++++- >> >>> 1 file changed, 61 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/platform/linux-generic/source/odp_timer.c >> >>> b/platform/linux-generic/source/odp_timer.c >> >>> index 4bcc479..890dffb 100644 >> >>> --- a/platform/linux-generic/source/odp_timer.c >> >>> +++ b/platform/linux-generic/source/odp_timer.c >> >>> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) >> >>> } >> >>> >> >>> >> >>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >> >>> +{ >> >>> + timeout_t *cur, *prev; >> >>> + prev = NULL; >> >>> + >> >>> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >> >>> + if (cur->tmo_buf == handle) >> >>> + break; >> >>> + } >> >>> + >> >>> + if (prev == NULL) >> >>> + tmo = cur->next; >> >> >> >> >> >> If tmo is the first item in the list, the list breaks here. This does >> not >> >> remove cur from the list, but sets only the local variable tmo. >> >> >> > >> > I disagree, implementation will remove middle/last and first from >> > list. if you have better solution then propose. >> >> >> >> I see that whole condition should move inside for loop i.e.. logic for >> deleting head node in list or middle / last node. Other than that I >> find logic pretty elegant to address first entry in list type of >> condition. pasting a modified code snippet before sending v4 patch. >> >> With few other concern on freeing tmo_buf and buf of timeout_t >> struct.. mentioned in below snippet as a question. >> >> --------- >> /* >> * function to search and delete tmo entry from timeout list >> * as well free tmo_buif, buf.. >> * return 0 : on error.. handle not in list >> * 1 : success >> */ >> >> static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >> { >> timeout_t *cur, *prev; >> prev = NULL; >> >> for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >> if (cur->tmo_buf == handle) { >> if (prev == NULL) >> tmo = cur->next; >> else >> prev->next = cur->next; >> >> break; >> } >> } >> >> if (!cur) { >> ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); >> return 0; >> } >> >> /* free tmo buf .. application to free */ >> odp_buffer_free(cur->tmo_buf); >> >> /* ideally delete operation should free tmo_buf, buf then list >> entry i.e.. cur >> though not sure appropriate api to delete cur entry from >> list, also conditional use-case >> for freeing buf of timeout_t..???*/ >> /*odp_buffer_free(cur->buf); >> >> odp_buffer_free(cur);*/ >> return 1; >> } >> ----------- >> >> >> >>> >> >>> >> >>> + else >> >>> + prev->next = cur->next; >> >>> + >> >>> + if (!cur) { >> >>> + ODP_ERR("Couldn't find the tmo handle (%d)\n", >> handle); >> >>> + return -1; >> >>> + } >> >>> + >> >>> + /* free tmo buf .. application to free */ >> >>> + >> >> >> >> >> >> Timer is responsible of the tmo_buf (that itself allocated). So that >> needs >> >> to be frees. Not here though, since it's better to unlock first and let >> >> other cores access the tick list. Return tmo_buf from this function. >> >> >> > >> > Why? tmo_buf very much accesible to application. .See ping_test >> > application therefore it make sense to let application free tmo_buf. >> > >> > Also, whats use case for keeping two buffers in timeout_t { It >> > confuses me,, when to allocate and free those buffers i.e.. buf and >> > tmo_buf from timeout_t { >> > >> >> Application is responsible of the (optional) tmo->buf that application >> >> allocated. >> >> >> >> -Petri >> >> >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> Groups >> >> "LNG ODP Sub-team - lng...@linaro.org <javascript:>" group. >> >> To unsubscribe from this group and stop receiving emails from it, send >> an >> >> email to lng-odp+u...@linaro.org <javascript:>. >> >> To post to this group, send email to lng...@linaro.org <javascript:>. >> >> Visit this group at >> http://groups.google.com/a/linaro.org/group/lng-odp/. >> >> To view this discussion on the web visit >> >> >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org >> . >> >> For more options, visit >> https://groups.google.com/a/linaro.org/d/optout. >> >> -- >> You received this message because you are subscribed to the Google Groups >> "LNG ODP Sub-team - lng...@linaro.org <javascript:>" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to lng-odp+u...@linaro.org <javascript:>. >> To post to this group, send email to lng...@linaro.org <javascript:>. >> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. >> To view this discussion on the web visit >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/CA%2BiXiiNL3wNz40JkPktV8k%3DXhd30vsPMY07%2BbgVqz%3DWZPKoD9Q%40mail.gmail.com >> . >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >> > >
Or did you abort a healthy discussion on use cases and requirements where we were lucky to have some of the LNG members' experts involved? If we at least had agreed on the API, you could contribute a 0th version of an implementation of that API that could then be incrementally improved. Now you contributed an implementation which also set the API, this makes changes much more complicated. You also set the standard process for developing new features, no discussion on requirements and design, just merge some code. Collaboration doesn't mean you are just using the same git repository for your code drops. On 2 April 2014 10:38, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Hi, > > I have told you this already - it's the first draft, everybody knows that > it's not complete. I put it there to kick start development and expected > Santosh, and maybe even you, to continue from that. Too near / too far / > etc errors / feature xyz needs to be added - it's not a big deal. If you > feel passionate - contribute code. > > And again, it's not complete implementation. It's crap - everybody knows > that. But it's more that word on mailing list - it actually delivers > timeouts today. Again, expected other guys to pick it up. > > Of course list search per cancel is crab. But again, what if the list > would a double-linked list. You have a handle to the tmo and can remove it > from the list with touching only three cache lines, which may be just good > enough (or even better than a more complicated structure). Note locking is > per tmo list and there are many tmos, average lock contention is low. > > -Petri > > > > On Tuesday, 1 April 2014 16:55:19 UTC+3, Ola Liljedahl wrote: > >> Two points. >> >> This timer API that was sneaked into ODP is flawed. E.g. you can get >> different errors (although it might not happen with this specific >> implementation) when arming/resetting/disarming timers which is done when a >> flow is already set up and you really really don't want to fail resetting a >> timer because that will cause you to tear down the flow (if you can't >> supervise it, you can't keep it or you might leak resources and be subject >> to DoS situations) and add a lot of complexity to the application. >> >> This implementation is also crap, we can't traverse over linked lists >> when looking for timers. How will that work with more than say 10 active >> timers? Even linux-generic is worthy a better implementation. Which I >> actually have and now is able to contribute. A timer implementation using a >> priority queue based on a 4-ary heap (all children of each node fit into >> the same 64-byte cache line). Seems to scale well to hundreds of thousand >> of timers, only a couple of cache lines accessed for each timer reset >> operation. >> >> So I suggest we don't waste more time on this current implementation. >> >> >> >> On 1 April 2014 11:39, Santosh Shukla <santosh...@linaro.org> wrote: >> >>> On 31 March 2014 12:16, Santosh Shukla <santosh...@linaro.org> wrote: >>> >>> > On 31 March 2014 02:22, Petri Savolainen <petri.sa...@linaro.org> >>> wrote: >>> >> >>> >> >>> >> On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: >>> >>> >>> >>> From: santosh shukla <santosh...@linaro.org> >>> >>> >>> >>> Signed-off-by: santosh shukla <santosh...@linaro.org> >>> >>> --- >>> >>> v2 change: >>> >>> - added find and delete function so to delete only >>> >>> tmo not the entire list. >>> >>> >>> >>> v3 change: >>> >>> - moved tmo_buf free to application layer. >>> >>> - changed serach and delete logic. >>> >>> >>> >>> platform/linux-generic/source/odp_timer.c | 63 >>> >>> ++++++++++++++++++++++++++++- >>> >>> 1 file changed, 61 insertions(+), 2 deletions(-) >>> >>> >>> >>> diff --git a/platform/linux-generic/source/odp_timer.c >>> >>> b/platform/linux-generic/source/odp_timer.c >>> >>> index 4bcc479..890dffb 100644 >>> >>> --- a/platform/linux-generic/source/odp_timer.c >>> >>> +++ b/platform/linux-generic/source/odp_timer.c >>> >>> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) >>> >>> } >>> >>> >>> >>> >>> >>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >>> >>> +{ >>> >>> + timeout_t *cur, *prev; >>> >>> + prev = NULL; >>> >>> + >>> >>> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >>> >>> + if (cur->tmo_buf == handle) >>> >>> + break; >>> >>> + } >>> >>> + >>> >>> + if (prev == NULL) >>> >>> + tmo = cur->next; >>> >> >>> >> >>> >> If tmo is the first item in the list, the list breaks here. This does >>> not >>> >> remove cur from the list, but sets only the local variable tmo. >>> >> >>> > >>> > I disagree, implementation will remove middle/last and first from >>> > list. if you have better solution then propose. >>> >> >>> >>> I see that whole condition should move inside for loop i.e.. logic for >>> deleting head node in list or middle / last node. Other than that I >>> find logic pretty elegant to address first entry in list type of >>> condition. pasting a modified code snippet before sending v4 patch. >>> >>> With few other concern on freeing tmo_buf and buf of timeout_t >>> struct.. mentioned in below snippet as a question. >>> >>> --------- >>> /* >>> * function to search and delete tmo entry from timeout list >>> * as well free tmo_buif, buf.. >>> * return 0 : on error.. handle not in list >>> * 1 : success >>> */ >>> >>> static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >>> { >>> timeout_t *cur, *prev; >>> prev = NULL; >>> >>> for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >>> if (cur->tmo_buf == handle) { >>> if (prev == NULL) >>> tmo = cur->next; >>> else >>> prev->next = cur->next; >>> >>> break; >>> } >>> } >>> >>> if (!cur) { >>> ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); >>> return 0; >>> } >>> >>> /* free tmo buf .. application to free */ >>> odp_buffer_free(cur->tmo_buf); >>> >>> /* ideally delete operation should free tmo_buf, buf then list >>> entry i.e.. cur >>> though not sure appropriate api to delete cur entry from >>> list, also conditional use-case >>> for freeing buf of timeout_t..???*/ >>> /*odp_buffer_free(cur->buf); >>> >>> odp_buffer_free(cur);*/ >>> return 1; >>> } >>> ----------- >>> >>> >>> >>> >>> >>> >>> >>> + else >>> >>> + prev->next = cur->next; >>> >>> + >>> >>> + if (!cur) { >>> >>> + ODP_ERR("Couldn't find the tmo handle (%d)\n", >>> handle); >>> >>> + return -1; >>> >>> + } >>> >>> + >>> >>> + /* free tmo buf .. application to free */ >>> >>> + >>> >> >>> >> >>> >> Timer is responsible of the tmo_buf (that itself allocated). So that >>> needs >>> >> to be frees. Not here though, since it's better to unlock first and >>> let >>> >> other cores access the tick list. Return tmo_buf from this function. >>> >> >>> > >>> > Why? tmo_buf very much accesible to application. .See ping_test >>> > application therefore it make sense to let application free tmo_buf. >>> > >>> > Also, whats use case for keeping two buffers in timeout_t { It >>> > confuses me,, when to allocate and free those buffers i.e.. buf and >>> > tmo_buf from timeout_t { >>> > >>> >> Application is responsible of the (optional) tmo->buf that application >>> >> allocated. >>> >> >>> >> -Petri >>> >> >>> >> >>> >> -- >>> >> You received this message because you are subscribed to the Google >>> Groups >>> >> "LNG ODP Sub-team - lng...@linaro.org" group. >>> >>> >> To unsubscribe from this group and stop receiving emails from it, >>> send an >>> >> email to lng-odp+u...@linaro.org. >>> >> To post to this group, send email to lng...@linaro.org. >>> >>> >> Visit this group at http://groups.google.com/a/ >>> linaro.org/group/lng-odp/. >>> >> To view this discussion on the web visit >>> >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/ >>> 9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org. >>> >> For more options, visit https://groups.google.com/a/ >>> linaro.org/d/optout. >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "LNG ODP Sub-team - lng...@linaro.org" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to lng-odp+u...@linaro.org. >>> To post to this group, send email to lng...@linaro.org. >>> >>> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/ >>> . >>> To view this discussion on the web visit https://groups.google.com/a/ >>> linaro.org/d/msgid/lng-odp/CA%2BiXiiNL3wNz40JkPktV8k% >>> 3DXhd30vsPMY07%2BbgVqz%3DWZPKoD9Q%40mail.gmail.com. >>> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >>> >> >> -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/5aa1269e-1b58-430f-80a5-0e6c5f0b0371%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/5aa1269e-1b58-430f-80a5-0e6c5f0b0371%40linaro.org?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
Hi, And why you cannot continue the discussion and extend on top of the current code? The main difference to the earlier API proposal was concept of multiple timers (vs. timer == timeout). HW supports multiple timers and that's now addressed. All the error codes, etc things are not there just to leave room to you and others to design/contribute those. I prefer multiple steps of working code, ported on multiple target before claiming an API a success. We cannot discuss months on things that people "think" may work, without actually implementing and proving it. E.g. you could continue the discussion by creating test applications illustrating different timer use cases and then modifications to the API/implementation to address those (instead of endless emails on the same subject). -Petri On Wednesday, 2 April 2014 12:59:52 UTC+3, Ola Liljedahl wrote: > > Or did you abort a healthy discussion on use cases and requirements where > we were lucky to have some of the LNG members' experts involved? > If we at least had agreed on the API, you could contribute a 0th version > of an implementation of that API that could then be incrementally improved. > Now you contributed an implementation which also set the API, this makes > changes much more complicated. You also set the standard process for > developing new features, no discussion on requirements and design, just > merge some code. Collaboration doesn't mean you are just using the same git > repository for your code drops. > > > On 2 April 2014 10:38, Petri Savolainen <petri.sa...@linaro.org<javascript:> > > wrote: > >> Hi, >> >> I have told you this already - it's the first draft, everybody knows that >> it's not complete. I put it there to kick start development and expected >> Santosh, and maybe even you, to continue from that. Too near / too far / >> etc errors / feature xyz needs to be added - it's not a big deal. If you >> feel passionate - contribute code. >> >> And again, it's not complete implementation. It's crap - everybody knows >> that. But it's more that word on mailing list - it actually delivers >> timeouts today. Again, expected other guys to pick it up. >> >> Of course list search per cancel is crab. But again, what if the list >> would a double-linked list. You have a handle to the tmo and can remove it >> from the list with touching only three cache lines, which may be just good >> enough (or even better than a more complicated structure). Note locking is >> per tmo list and there are many tmos, average lock contention is low. >> >> -Petri >> >> >> >> On Tuesday, 1 April 2014 16:55:19 UTC+3, Ola Liljedahl wrote: >> >>> Two points. >>> >>> This timer API that was sneaked into ODP is flawed. E.g. you can get >>> different errors (although it might not happen with this specific >>> implementation) when arming/resetting/disarming timers which is done when a >>> flow is already set up and you really really don't want to fail resetting a >>> timer because that will cause you to tear down the flow (if you can't >>> supervise it, you can't keep it or you might leak resources and be subject >>> to DoS situations) and add a lot of complexity to the application. >>> >>> This implementation is also crap, we can't traverse over linked lists >>> when looking for timers. How will that work with more than say 10 active >>> timers? Even linux-generic is worthy a better implementation. Which I >>> actually have and now is able to contribute. A timer implementation using a >>> priority queue based on a 4-ary heap (all children of each node fit into >>> the same 64-byte cache line). Seems to scale well to hundreds of thousand >>> of timers, only a couple of cache lines accessed for each timer reset >>> operation. >>> >>> So I suggest we don't waste more time on this current implementation. >>> >>> >>>
Can we have a private discussion on Timer Implementation some time today. Bala, schedule a HO, hopefully we can only make better progress if we all talk real time. Mike On 2 April 2014 06:24, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Hi, > > And why you cannot continue the discussion and extend on top of the > current code? The main difference to the earlier API proposal was concept > of multiple timers (vs. timer == timeout). HW supports multiple timers and > that's now addressed. All the error codes, etc things are not there just to > leave room to you and others to design/contribute those. I prefer multiple > steps of working code, ported on multiple target before claiming an API a > success. We cannot discuss months on things that people "think" may work, > without actually implementing and proving it. > > E.g. you could continue the discussion by creating test applications > illustrating different timer use cases and then modifications to the > API/implementation to address those (instead of endless emails on the same > subject). > > -Petri > > > > On Wednesday, 2 April 2014 12:59:52 UTC+3, Ola Liljedahl wrote: > >> Or did you abort a healthy discussion on use cases and requirements where >> we were lucky to have some of the LNG members' experts involved? >> If we at least had agreed on the API, you could contribute a 0th version >> of an implementation of that API that could then be incrementally improved. >> Now you contributed an implementation which also set the API, this makes >> changes much more complicated. You also set the standard process for >> developing new features, no discussion on requirements and design, just >> merge some code. Collaboration doesn't mean you are just using the same git >> repository for your code drops. >> >> >> On 2 April 2014 10:38, Petri Savolainen <petri.sa...@linaro.org> wrote: >> >>> Hi, >>> >>> I have told you this already - it's the first draft, everybody knows >>> that it's not complete. I put it there to kick start development and >>> expected Santosh, and maybe even you, to continue from that. Too near / too >>> far / etc errors / feature xyz needs to be added - it's not a big deal. If >>> you feel passionate - contribute code. >>> >>> And again, it's not complete implementation. It's crap - everybody knows >>> that. But it's more that word on mailing list - it actually delivers >>> timeouts today. Again, expected other guys to pick it up. >>> >>> Of course list search per cancel is crab. But again, what if the list >>> would a double-linked list. You have a handle to the tmo and can remove it >>> from the list with touching only three cache lines, which may be just good >>> enough (or even better than a more complicated structure). Note locking is >>> per tmo list and there are many tmos, average lock contention is low. >>> >>> -Petri >>> >>> >>> >>> On Tuesday, 1 April 2014 16:55:19 UTC+3, Ola Liljedahl wrote: >>> >>>> Two points. >>>> >>>> This timer API that was sneaked into ODP is flawed. E.g. you can get >>>> different errors (although it might not happen with this specific >>>> implementation) when arming/resetting/disarming timers which is done when a >>>> flow is already set up and you really really don't want to fail resetting a >>>> timer because that will cause you to tear down the flow (if you can't >>>> supervise it, you can't keep it or you might leak resources and be subject >>>> to DoS situations) and add a lot of complexity to the application. >>>> >>>> This implementation is also crap, we can't traverse over linked lists >>>> when looking for timers. How will that work with more than say 10 active >>>> timers? Even linux-generic is worthy a better implementation. Which I >>>> actually have and now is able to contribute. A timer implementation using a >>>> priority queue based on a 4-ary heap (all children of each node fit into >>>> the same 64-byte cache line). Seems to scale well to hundreds of thousand >>>> of timers, only a couple of cache lines accessed for each timer reset >>>> operation. >>>> >>>> So I suggest we don't waste more time on this current implementation. >>>> >>>> >>>> -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/91706c5e-0f01-407a-9cfa-e3b43f00f308%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/91706c5e-0f01-407a-9cfa-e3b43f00f308%40linaro.org?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On Tuesday, 1 April 2014 12:39:11 UTC+3, Santosh Shukla wrote: > On 31 March 2014 12:16, Santosh Shukla <santosh...@linaro.org<javascript:>> > wrote: > > On 31 March 2014 02:22, Petri Savolainen <petri.sa...@linaro.org<javascript:>> > wrote: > >> > >> > >> On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: > >>> > >>> From: santosh shukla <santosh...@linaro.org> > >>> > >>> Signed-off-by: santosh shukla <santosh...@linaro.org> > >>> --- > >>> v2 change: > >>> - added find and delete function so to delete only > >>> tmo not the entire list. > >>> > >>> v3 change: > >>> - moved tmo_buf free to application layer. > >>> - changed serach and delete logic. > >>> > >>> platform/linux-generic/source/odp_timer.c | 63 > >>> ++++++++++++++++++++++++++++- > >>> 1 file changed, 61 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/platform/linux-generic/source/odp_timer.c > >>> b/platform/linux-generic/source/odp_timer.c > >>> index 4bcc479..890dffb 100644 > >>> --- a/platform/linux-generic/source/odp_timer.c > >>> +++ b/platform/linux-generic/source/odp_timer.c > >>> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) > >>> } > >>> > >>> > >>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) > >>> +{ > >>> + timeout_t *cur, *prev; > >>> + prev = NULL; > >>> + > >>> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { > >>> + if (cur->tmo_buf == handle) > >>> + break; > >>> + } > >>> + > >>> + if (prev == NULL) > >>> + tmo = cur->next; > >> > >> > >> If tmo is the first item in the list, the list breaks here. This does > not > >> remove cur from the list, but sets only the local variable tmo. > >> > > > > I disagree, implementation will remove middle/last and first from > > list. if you have better solution then propose. > >> > > I see that whole condition should move inside for loop i.e.. logic for > deleting head node in list or middle / last node. Other than that I > find logic pretty elegant to address first entry in list type of > condition. pasting a modified code snippet before sending v4 patch. > > With few other concern on freeing tmo_buf and buf of timeout_t > struct.. mentioned in below snippet as a question. > > --------- > /* > * function to search and delete tmo entry from timeout list > * as well free tmo_buif, buf.. > * return 0 : on error.. handle not in list > * 1 : success > */ > > static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) > { > timeout_t *cur, *prev; > prev = NULL; > > for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { > if (cur->tmo_buf == handle) { > if (prev == NULL) > tmo = cur->next; Here we found the timeout first in the list. Next you set the _local variable_ tmo to cur->next, instead of the list head. Variable tmo is a copy of the pointer to the first item in the list. From here on, the list points always to the timeout we are about to cancel and free the buffer. After buffer is freed and reused for some thing else, the list explodes. You should write tick->list, instead the copy of its contents. > > else > prev->next = cur->next; > > break; > } > } > > if (!cur) { > ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); > return 0; > } > > /* free tmo buf .. application to free */ > odp_buffer_free(cur->tmo_buf); Buffer_free has also a spinlock inside. So, it's better to return from here first, unlock the tick list and free buffer after that. Application will free the buffer (cur->buf) it provided in absolute_tmo call. -Petri > > > /* ideally delete operation should free tmo_buf, buf then list > entry i.e.. cur > though not sure appropriate api to delete cur entry from > list, also conditional use-case > for freeing buf of timeout_t..???*/ > /*odp_buffer_free(cur->buf); > > odp_buffer_free(cur);*/ > return 1; > } > ----------- > > > >>> > >>> > >>> + else > >>> + prev->next = cur->next; > >>> + > >>> + if (!cur) { > >>> + ODP_ERR("Couldn't find the tmo handle (%d)\n", > handle); > >>> + return -1; > >>> + } > >>> + > >>> + /* free tmo buf .. application to free */ > >>> + > >> > >> > >> Timer is responsible of the tmo_buf (that itself allocated). So that > needs > >> to be frees. Not here though, since it's better to unlock first and let > >> other cores access the tick list. Return tmo_buf from this function. > >> > > > > Why? tmo_buf very much accesible to application. .See ping_test > > application therefore it make sense to let application free tmo_buf. > > > > Also, whats use case for keeping two buffers in timeout_t { It > > confuses me,, when to allocate and free those buffers i.e.. buf and > > tmo_buf from timeout_t { > > > >> Application is responsible of the (optional) tmo->buf that application > >> allocated. > >> > >> -Petri > >> > >> > >> -- > >> You received this message because you are subscribed to the Google > Groups > >> "LNG ODP Sub-team - lng...@linaro.org <javascript:>" group. > >> To unsubscribe from this group and stop receiving emails from it, send > an > >> email to lng-odp+u...@linaro.org <javascript:>. > >> To post to this group, send email to lng...@linaro.org <javascript:>. > >> Visit this group at > http://groups.google.com/a/linaro.org/group/lng-odp/. > >> To view this discussion on the web visit > >> > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org. > > >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. > >
On 3 April 2014 19:14, Petri Savolainen <petri.savolainen@linaro.org> wrote: > > > On Tuesday, 1 April 2014 12:39:11 UTC+3, Santosh Shukla wrote: >> >> On 31 March 2014 12:16, Santosh Shukla <santosh...@linaro.org> wrote: >> > On 31 March 2014 02:22, Petri Savolainen <petri.sa...@linaro.org> wrote: >> >> >> >> >> >> On Saturday, 29 March 2014 04:56:29 UTC+2, Santosh Shukla wrote: >> >>> >> >>> From: santosh shukla <santosh...@linaro.org> >> >>> >> >>> Signed-off-by: santosh shukla <santosh...@linaro.org> >> >>> --- >> >>> v2 change: >> >>> - added find and delete function so to delete only >> >>> tmo not the entire list. >> >>> >> >>> v3 change: >> >>> - moved tmo_buf free to application layer. >> >>> - changed serach and delete logic. >> >>> >> >>> platform/linux-generic/source/odp_timer.c | 63 >> >>> ++++++++++++++++++++++++++++- >> >>> 1 file changed, 61 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/platform/linux-generic/source/odp_timer.c >> >>> b/platform/linux-generic/source/odp_timer.c >> >>> index 4bcc479..890dffb 100644 >> >>> --- a/platform/linux-generic/source/odp_timer.c >> >>> +++ b/platform/linux-generic/source/odp_timer.c >> >>> @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) >> >>> } >> >>> >> >>> >> >>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >> >>> +{ >> >>> + timeout_t *cur, *prev; >> >>> + prev = NULL; >> >>> + >> >>> + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >> >>> + if (cur->tmo_buf == handle) >> >>> + break; >> >>> + } >> >>> + >> >>> + if (prev == NULL) >> >>> + tmo = cur->next; >> >> >> >> >> >> If tmo is the first item in the list, the list breaks here. This does >> >> not >> >> remove cur from the list, but sets only the local variable tmo. >> >> >> > >> > I disagree, implementation will remove middle/last and first from >> > list. if you have better solution then propose. >> >> >> >> I see that whole condition should move inside for loop i.e.. logic for >> deleting head node in list or middle / last node. Other than that I >> find logic pretty elegant to address first entry in list type of >> condition. pasting a modified code snippet before sending v4 patch. >> >> With few other concern on freeing tmo_buf and buf of timeout_t >> struct.. mentioned in below snippet as a question. >> >> --------- >> /* >> * function to search and delete tmo entry from timeout list >> * as well free tmo_buif, buf.. >> * return 0 : on error.. handle not in list >> * 1 : success >> */ >> >> static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) >> { >> timeout_t *cur, *prev; >> prev = NULL; >> >> for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { >> if (cur->tmo_buf == handle) { >> if (prev == NULL) >> tmo = cur->next; > > > Here we found the timeout first in the list. Next you set the _local > variable_ tmo to cur->next, instead of the list head. Variable tmo is a copy > of the pointer to the first item in the list. From here on, the list points > always to the timeout we are about to cancel and free the buffer. After > buffer is freed and reused for some thing else, the list explodes. > > You should write tick->list, instead the copy of its contents. > Ah!, its a stupid programming mistake, I didn't noticed while pasting a snap,yes you rightly pointed out that node deletion happening in local copy, correct code should has double pointer. static int find_and_del_tmo(timeout_t **tmo, odp_timer_tmo_t handle) { ... ... for (cur = *tmo; cur != NULL; prev = cur, cur = cur->next) { if (cur->tmo_buf == handle) { if (prev == NULL) *tmo = cur->next; >> >> >> else >> prev->next = cur->next; >> >> break; >> } >> } >> >> if (!cur) { >> ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); >> return 0; >> } >> >> /* free tmo buf .. application to free */ >> odp_buffer_free(cur->tmo_buf); > > > Buffer_free has also a spinlock inside. So, it's better to return from here > first, unlock the tick list and free buffer after that. Application will > free the buffer (cur->buf) it provided in absolute_tmo call. > Yes and I am aware of that tmo_buf, apllication to free, timer_ping application does that, Its just a snap to ask question on - timeout_t { odp_buffet_t buf} use-case, regrading who will populate, why do we need since we already have tmo_buf. I dont see in your implementation buf used but you do care to check buf in handle notify_function() ; static void notify_function (sigval) { ... ... if (buf != tmo->tmo_buf) odp_buffer_free(tmo->tmo_buf); .. .. } > -Petri > >> >> >> >> /* ideally delete operation should free tmo_buf, buf then list >> entry i.e.. cur >> though not sure appropriate api to delete cur entry from >> list, also conditional use-case >> for freeing buf of timeout_t..???*/ >> /*odp_buffer_free(cur->buf); >> >> odp_buffer_free(cur);*/ >> return 1; >> } >> ----------- >> >> >> >>> >> >>> >> >>> + else >> >>> + prev->next = cur->next; >> >>> + >> >>> + if (!cur) { >> >>> + ODP_ERR("Couldn't find the tmo handle (%d)\n", >> >>> handle); >> >>> + return -1; >> >>> + } >> >>> + >> >>> + /* free tmo buf .. application to free */ >> >>> + >> >> >> >> >> >> Timer is responsible of the tmo_buf (that itself allocated). So that >> >> needs >> >> to be frees. Not here though, since it's better to unlock first and let >> >> other cores access the tick list. Return tmo_buf from this function. >> >> >> > >> > Why? tmo_buf very much accesible to application. .See ping_test >> > application therefore it make sense to let application free tmo_buf. >> > >> > Also, whats use case for keeping two buffers in timeout_t { It >> > confuses me,, when to allocate and free those buffers i.e.. buf and >> > tmo_buf from timeout_t { >> > >> >> Application is responsible of the (optional) tmo->buf that application >> >> allocated. >> >> >> >> -Petri >> >> >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> >> Groups >> >> "LNG ODP Sub-team - lng...@linaro.org" group. >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> >> email to lng-odp+u...@linaro.org. >> >> To post to this group, send email to lng...@linaro.org. >> >> Visit this group at >> >> http://groups.google.com/a/linaro.org/group/lng-odp/. >> >> To view this discussion on the web visit >> >> >> >> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/9f5c8b74-771d-423f-894a-9510bcf4698d%40linaro.org. >> >> For more options, visit >> >> https://groups.google.com/a/linaro.org/d/optout. > > -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/b398cb57-557f-4fd5-89c2-3a43422532c3%40linaro.org. > > For more options, visit https://groups.google.com/a/linaro.org/d/optout.
diff --git a/platform/linux-generic/source/odp_timer.c b/platform/linux-generic/source/odp_timer.c index 4bcc479..890dffb 100644 --- a/platform/linux-generic/source/odp_timer.c +++ b/platform/linux-generic/source/odp_timer.c @@ -92,6 +92,67 @@ static timeout_t *rem_tmo(tick_t *tick) } +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) +{ + timeout_t *cur, *prev; + prev = NULL; + + for (cur = tmo; cur != NULL; prev = cur, cur = cur->next) { + if (cur->tmo_buf == handle) + break; + } + + if (prev == NULL) + tmo = cur->next; + else + prev->next = cur->next; + + if (!cur) { + ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); + return -1; + } + + /* free tmo buf .. application to free */ + + return 0; +} + + +/** + * Cancel a timeout + * + * @param timer Timer + * @param tmo Timeout to cancel + * + * @return 0 if successful + */ +int odp_timer_cancel_tmo(odp_timer_t timer, odp_timer_tmo_t tmo) +{ + int id; + uint64_t tick_idx; + timeout_t *cancel_tmo; + tick_t *tick; + + /* get id */ + id = timer - 1; + + /* get tmo_buf to cancel */ + cancel_tmo = (timeout_t *)odp_buffer_addr(tmo); + tick_idx = cancel_tmo->tick; + tick = &odp_timer.timer[id].tick[tick_idx]; + + odp_spinlock_lock(&tick->lock); + /* search and delete tmo from tick list */ + if (find_and_del_tmo(tick->list, tmo) != 0) { + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, tmo, tick_idx); + odp_spinlock_unlock(&tick->lock); + return -1; + } + odp_spinlock_unlock(&tick->lock); + + return 0; +} + static void notify_function(union sigval sigval) { @@ -167,8 +228,6 @@ int odp_timer_init_global(void) odp_spinlock_init(&odp_timer.timer[0].tick[i].lock); timer_init(); - - return 0; }