Re: [Minios-devel] [UNIKRAFT PATCH 1/8] lib/uknetdev: Destroy dispatcher thread using public scheduling API

[Costin Lupu <costin.lupu@xxxxxxxxx>]

I don't see the comment issue being related with this patch (the comment
is internal to the cooperative scheduler, so from the point of view of
an API user it shouldn't make any difference). It's better to remove it
in another patch.

Costin

On 5/16/19 2:23 PM, Florian Schmidt wrote:
> OK, I see, then this is indeed fine. Do you think though to remove that
> outdated comment then? You could in theory just do it with this patch
> and quickly point out in the comment why that is an incorrect comment,
> which at the same time clarifies this one?
> 
> Reviewed-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> 
> On 5/16/19 11:17 AM, Costin Lupu wrote:
>> Hi Florian,
>>
>> In deed, the comment "Schedule will free resources" is outdated since we
>> introduced the thread waiting function. So, in order to free thread
>> resources one should either call uk_thread_wait() or set the thread as
>> detached in order to delegate the free operation to the scheduler as
>> before.
>>
>> Costin
>>
>> On 5/16/19 11:03 AM, Florian Schmidt wrote:
>>> Hi Costin,
>>>
>>> the patch looks right in its concept. I'm just wondering: is the
>>> uk_thread_wait actually necessary and wanted at this stage? It seems
>>> uk_thread_kill() calls uk_sched_thread_remove(), which calls
>>> ukschedcoop_thread_remove() (or in theory any other implementation), and
>>> that one runs a scheduler loop with the comment "Schedule will free
>>> resources". So isn't uk_thread_kill(h->dispatcher) already enough?
>>>
>>> Cheers,
>>> Florian
>>>
>>> On 4/23/19 12:41 PM, Costin Lupu wrote:
>>>> When thread destruction is wanted, one should kill the thread and wait
>>>> for it in order to free its resources. uk_sched_thread_destroy() is not
>>>> part of public scheduling API.
>>>>
>>>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>>>> ---
>>>>    lib/uknetdev/netdev.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
>>>> index cda8a82b..90ecd0a4 100644
>>>> --- a/lib/uknetdev/netdev.c
>>>> +++ b/lib/uknetdev/netdev.c
>>>> @@ -311,8 +311,10 @@ static void _destroy_event_handler(struct
>>>> uk_netdev_event_handler *h
>>>>    #ifdef CONFIG_LIBUKNETDEV_DISPATCHERTHREADS
>>>>        UK_ASSERT(h->dispatcher_s);
>>>>    -    if (h->dispatcher)
>>>> -        uk_sched_thread_destroy(h->dispatcher_s, h->dispatcher);
>>>> +    if (h->dispatcher) {
>>>> +        uk_thread_kill(h->dispatcher);
>>>> +        uk_thread_wait(h->dispatcher);
>>>> +    }
>>>>        h->dispatcher = NULL;
>>>>          if (h->dispatcher_name)
>>>>
>>>
> 

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