| Message ID | 1470645629-12411-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Accepted |
| Commit | 26b90314427f088c7f7e2accd41ec7db5d396265 |
| Headers | show |
I can't say whether this fixes Bug 2457, but this certainly seems like a bug worth correcting to me. Would like Petri's opinion on this. But in the meantime... On Mon, Aug 8, 2016 at 3:40 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > We should not try to redifine current odp thread id on > termination as well as set poll queues to invalid values, > which has to be cleared later in schedule_term_global(). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/odp_schedule.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/platform/linux-generic/odp_schedule.c > b/platform/linux-generic/odp_schedule.c > index 8765f48..8405423 100644 > --- a/platform/linux-generic/odp_schedule.c > +++ b/platform/linux-generic/odp_schedule.c > @@ -328,8 +328,6 @@ static int schedule_term_local(void) > } > > schedule_release_context(); > - > - sched_local_init(); > return 0; > } > > -- > 2.7.1.250.gff4ea60 > >
On 08/08 11:40:29, Maxim Uvarov wrote: > We should not try to redifine current odp thread id on > termination as well as set poll queues to invalid values, > which has to be cleared later in schedule_term_global(). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/odp_schedule.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c > index 8765f48..8405423 100644 > --- a/platform/linux-generic/odp_schedule.c > +++ b/platform/linux-generic/odp_schedule.c > @@ -328,8 +328,6 @@ static int schedule_term_local(void) > } > > schedule_release_context(); > - > - sched_local_init(); > return 0; > } Perhaps there is harm in reinitializing these TLS vars here, but I don't see the connection to the scheduling bug seen in odp_scheduling program since the issue is seen when attempting to destroy non-empty queues which happens before the schedule_term_local() call. Maybe a way to test this particular change is if there was a program which started worker threads, did work, stopped the threads, and then did the same thing again? More dynamic thread lifetime within single program run.
On Tue, Aug 9, 2016 at 12:03 PM, Brian Brooks <brian.brooks@linaro.org> wrote: > On 08/08 11:40:29, Maxim Uvarov wrote: > > We should not try to redifine current odp thread id on > > termination as well as set poll queues to invalid values, > > which has to be cleared later in schedule_term_global(). > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > > --- > > platform/linux-generic/odp_schedule.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/platform/linux-generic/odp_schedule.c > b/platform/linux-generic/odp_schedule.c > > index 8765f48..8405423 100644 > > --- a/platform/linux-generic/odp_schedule.c > > +++ b/platform/linux-generic/odp_schedule.c > > @@ -328,8 +328,6 @@ static int schedule_term_local(void) > > } > > > > schedule_release_context(); > > - > > - sched_local_init(); > > return 0; > > } > > Perhaps there is harm in reinitializing these TLS vars here, but I don't > see the connection to the scheduling bug seen in odp_scheduling program > since the issue is seen when attempting to destroy non-empty queues which > happens before the schedule_term_local() call. > So are you saying you still see Bug 2457 with this patch applied? > > Maybe a way to test this particular change is if there was a program which > started worker threads, did work, stopped the threads, and then did the > same thing again? More dynamic thread lifetime within single program run. >
On 08/09 19:00:32, Bill Fischofer wrote: > On Tue, Aug 9, 2016 at 12:03 PM, Brian Brooks <brian.brooks@linaro.org> > wrote: > > > On 08/08 11:40:29, Maxim Uvarov wrote: > > > We should not try to redifine current odp thread id on > > > termination as well as set poll queues to invalid values, > > > which has to be cleared later in schedule_term_global(). > > > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > > > --- > > > platform/linux-generic/odp_schedule.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/platform/linux-generic/odp_schedule.c > > b/platform/linux-generic/odp_schedule.c > > > index 8765f48..8405423 100644 > > > --- a/platform/linux-generic/odp_schedule.c > > > +++ b/platform/linux-generic/odp_schedule.c > > > @@ -328,8 +328,6 @@ static int schedule_term_local(void) > > > } > > > > > > schedule_release_context(); > > > - > > > - sched_local_init(); > > > return 0; > > > } > > > > Perhaps there is harm in reinitializing these TLS vars here, but I don't > > see the connection to the scheduling bug seen in odp_scheduling program > > since the issue is seen when attempting to destroy non-empty queues which > > happens before the schedule_term_local() call. > > > > So are you saying you still see Bug 2457 with this patch applied? Yes. Bug 2457 is seen with this patch applied. > > > > Maybe a way to test this particular change is if there was a program which > > started worker threads, did work, stopped the threads, and then did the > > same thing again? More dynamic thread lifetime within single program run.
On 08/10/16 05:38, Brian Brooks wrote: > On 08/09 19:00:32, Bill Fischofer wrote: >> On Tue, Aug 9, 2016 at 12:03 PM, Brian Brooks <brian.brooks@linaro.org> >> wrote: >> >>> On 08/08 11:40:29, Maxim Uvarov wrote: >>>> We should not try to redifine current odp thread id on >>>> termination as well as set poll queues to invalid values, >>>> which has to be cleared later in schedule_term_global(). >>>> >>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >>>> --- >>>> platform/linux-generic/odp_schedule.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/odp_schedule.c >>> b/platform/linux-generic/odp_schedule.c >>>> index 8765f48..8405423 100644 >>>> --- a/platform/linux-generic/odp_schedule.c >>>> +++ b/platform/linux-generic/odp_schedule.c >>>> @@ -328,8 +328,6 @@ static int schedule_term_local(void) >>>> } >>>> >>>> schedule_release_context(); >>>> - >>>> - sched_local_init(); >>>> return 0; >>>> } >>> Perhaps there is harm in reinitializing these TLS vars here, but I don't >>> see the connection to the scheduling bug seen in odp_scheduling program >>> since the issue is seen when attempting to destroy non-empty queues which >>> happens before the schedule_term_local() call. >>> >> So are you saying you still see Bug 2457 with this patch applied? > Yes. Bug 2457 is seen with this patch applied. Interesting. But without process mode this patch repairs odp_scheduling. At least for my local testing. Might be for process mode we need something else.... I think we can apply this patch and Bills patch which adds return codes to make check, to not hide the problem. Then repair proc mode. Btw, Brian, can you reproduce problem with SP scheduler? (--enable-sp-scheduler or something like that.). I.e. if you can not that problem definitely in scheduler code. Thank you, Maxim. > >>> Maybe a way to test this particular change is if there was a program which >>> started worker threads, did work, stopped the threads, and then did the >>> same thing again? More dynamic thread lifetime within single program run.
Merged, Maxim. On 08/10/16 05:38, Brian Brooks wrote: > On 08/09 19:00:32, Bill Fischofer wrote: >> On Tue, Aug 9, 2016 at 12:03 PM, Brian Brooks <brian.brooks@linaro.org> >> wrote: >> >>> On 08/08 11:40:29, Maxim Uvarov wrote: >>>> We should not try to redifine current odp thread id on >>>> termination as well as set poll queues to invalid values, >>>> which has to be cleared later in schedule_term_global(). >>>> >>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >>>> --- >>>> platform/linux-generic/odp_schedule.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/odp_schedule.c >>> b/platform/linux-generic/odp_schedule.c >>>> index 8765f48..8405423 100644 >>>> --- a/platform/linux-generic/odp_schedule.c >>>> +++ b/platform/linux-generic/odp_schedule.c >>>> @@ -328,8 +328,6 @@ static int schedule_term_local(void) >>>> } >>>> >>>> schedule_release_context(); >>>> - >>>> - sched_local_init(); >>>> return 0; >>>> } >>> Perhaps there is harm in reinitializing these TLS vars here, but I don't >>> see the connection to the scheduling bug seen in odp_scheduling program >>> since the issue is seen when attempting to destroy non-empty queues which >>> happens before the schedule_term_local() call. >>> >> So are you saying you still see Bug 2457 with this patch applied? > Yes. Bug 2457 is seen with this patch applied. > >>> Maybe a way to test this particular change is if there was a program which >>> started worker threads, did work, stopped the threads, and then did the >>> same thing again? More dynamic thread lifetime within single program run.
diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c index 8765f48..8405423 100644 --- a/platform/linux-generic/odp_schedule.c +++ b/platform/linux-generic/odp_schedule.c @@ -328,8 +328,6 @@ static int schedule_term_local(void) } schedule_release_context(); - - sched_local_init(); return 0; }
We should not try to redifine current odp thread id on termination as well as set poll queues to invalid values, which has to be cleared later in schedule_term_global(). Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/odp_schedule.c | 2 -- 1 file changed, 2 deletions(-) -- 2.7.1.250.gff4ea60