Re: [Minios-devel] [UNIKRAFT PATCH v2 5/8] lib/uksched: Add support for waiting threads

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Costin,

On 2/21/19 12:46 PM, Costin Lupu wrote:
> Hi Florian,
>
> Please see my comments inline.
>
> On 1/23/19 3:48 PM, Florian Schmidt wrote:
> > Actually, now that I look at the next patch...
> >
> > On 1/11/19 12:22 AM, Costin Lupu wrote:
> >
> > > diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
> > > index e565240..f7ab92d 100644
> > > --- a/lib/ukschedcoop/schedcoop.c
> > > +++ b/lib/ukschedcoop/schedcoop.c
> > > @@ -125,6 +125,12 @@ static void schedcoop_schedule(struct uk_sched *s)
> > >            uk_sched_thread_switch(s, prev, next);
> > >          UK_TAILQ_FOREACH_SAFE(thread, &prv->exited_threads,
> > > thread_list, tmp) {
> > > +        struct thread_info_base *tib = thread->sched_info;
> > > +
> > > +        if (!tib->is_detached)
> > > +            /* someone will eventually wait for it */
> > > +            continue;
> > > +
> > >            if (thread != prev) {
> > >                UK_TAILQ_REMOVE(&prv->exited_threads,
> > >                        thread, thread_list);
> > > @@ -167,6 +173,7 @@ static void schedcoop_thread_remove(struct
> > > uk_sched *s, struct uk_thread *t)
> > >          /* Put onto exited list */
> > >        UK_TAILQ_INSERT_HEAD(&prv->exited_threads, t, thread_list);
> > > +    uk_thread_exit(t);
> > >          ukplat_lcpu_restore_irqf(flags);
> > Doesn't calling uk_thread_exit(t) only after putting it on the
> > exited_threads list a race condition? Because if now the main scheduling
> > loop (as in schedcoop_schedule() is called again after inserting, but
> > before thread_exit() is called, the could lead to an assertion failure
> > when schedcoop_schedule() calls uk_sched_thread_destroy() on that
> > thread, but uk_thread_exit() hasn't finished yet.
> Will move this before the insertion in v3.
Great, thanks!


> > I understand that this probably is a crazy corner case, and I'm not sure
> > I can even ever occur without SMP support, but, on the other hand: is
> > there any harm in switching the order? And actually, wouldn't it make
> > sense to put the list insertion into uk_thread_exit()? It's an important
> > part of thread handling, so it could just be done in there, right?
> > Although I realize that you only do this in the next patch, so... maybe
> > only do that in there? Seems regardless of the order, one thing from the
> > earlier patch always better waits until the latter patch.
> uk_thread_exit() is a thread 'method', while the exited_threads list
> belongs to the scheduler. Since the thread doesn't have any kind of
> ownership of the exited_threads list, it's better to not move the
> insertion in uk_thread_list().
Alright, then let's just keep it as it is in this patch.

Cheers,
Florian

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel