|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 4/8] lib/uksched: Introduce thread info
Hi Felipe,
Please see my comments inline.
On 9/25/18 1:09 PM, Felipe Huici wrote:
> Hi,
>
>
>> Thread behaviors can be configured by using thread attributes. For
>> keeping this information, we introduce the thread info abstraction.
>> Schedulers should extend this abstraction when adding custom
>> configuration features.
>
> s/behaviors/behavior
> s/For keeping/To keep
>
> Perhaps add a sentence or two more to explain what “extend” means, e.g.,
> should a scheduler use thread_info_base_create or thread_info_base_init?
>
Ack.
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>> lib/uksched/Makefile.uk | 1 +
>> lib/uksched/exportsyms.uk | 3 ++
>> lib/uksched/include/uk/_thread_info.h | 57 +++++++++++++++++++++++++
>> lib/uksched/include/uk/sched.h | 6 +--
>> lib/uksched/include/uk/thread.h | 1 +
>> lib/uksched/sched.c | 8 +++-
>> lib/uksched/thread.c | 2 +
>> lib/uksched/thread_info.c | 79
>> +++++++++++++++++++++++++++++++++++
>> lib/ukschedcoop/schedcoop.c | 13 +++++-
>> 9 files changed, 164 insertions(+), 6 deletions(-)
>> create mode 100644 lib/uksched/include/uk/_thread_info.h
>> create mode 100644 lib/uksched/thread_info.c
>>
>> diff --git a/lib/uksched/Makefile.uk b/lib/uksched/Makefile.uk
>> index 229d847..76fea97 100644
>> --- a/lib/uksched/Makefile.uk
>> +++ b/lib/uksched/Makefile.uk
>> @@ -6,3 +6,4 @@ CXXINCLUDES-$(CONFIG_LIBUKSCHED) +=
>> -I$(LIBUKSCHED_BASE)/include
>> LIBUKSCHED_SRCS-y += $(LIBUKSCHED_BASE)/sched.c
>> LIBUKSCHED_SRCS-y += $(LIBUKSCHED_BASE)/thread.c
>> LIBUKSCHED_SRCS-y += $(LIBUKSCHED_BASE)/thread_attr.c
>> +LIBUKSCHED_SRCS-y += $(LIBUKSCHED_BASE)/thread_info.c
>> diff --git a/lib/uksched/exportsyms.uk b/lib/uksched/exportsyms.uk
>> index 7d8e102..951cd20 100644
>> --- a/lib/uksched/exportsyms.uk
>> +++ b/lib/uksched/exportsyms.uk
>> @@ -26,3 +26,6 @@ uk_thread_attr_set_prio
>> uk_thread_attr_get_prio
>> uk_thread_attr_set_timeslice
>> uk_thread_attr_get_timeslice
>> +thread_info_base_create
>> +thread_info_base_destroy
>> +thread_info_base_init
>> diff --git a/lib/uksched/include/uk/_thread_info.h
>> b/lib/uksched/include/uk/_thread_info.h
>> new file mode 100644
>> index 0000000..d7050d7
>> --- /dev/null
>> +++ b/lib/uksched/include/uk/_thread_info.h
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in the
>> + * documentation and/or other materials provided with the
>> distribution.
>> + * 3. Neither the name of the copyright holder nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> from
>> + * this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> "AS IS"
>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>> THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> PURPOSE
>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>> CONTRIBUTORS BE
>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> BUSINESS
>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
>> IN
>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>> OTHERWISE)
>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
>> OF THE
>> + * POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>> + */
>> +
>> +#ifndef __UK_SCHED__THREAD_INFO_H__
>> +#define __UK_SCHED__THREAD_INFO_H__
>> +
>> +#include <uk/alloc.h>
>> +#include <uk/thread_attr.h>
>
> Maybe you should #include <uk/thread.h> here? When running a dummy
> application
> I wrote that uses this code I had to add this include in it in order for
> it
> to compile.
>
This abstraction is internal to schedulers logic and should not be used
outside the scheduler libraries.
>> +
>> +struct thread_info_base {
>> + int is_detached;
>> +};
>> +
>> +struct thread_info_base *
>> +thread_info_base_create(struct uk_alloc *a,
>> + const uk_thread_attr_t *attr);
>> +
>> +void
>> +thread_info_base_destroy(struct uk_alloc *a,
>> + struct thread_info_base *tib);
>> +
>> +void
>> +thread_info_base_init(struct thread_info_base *tib,
>> + const uk_thread_attr_t *attr);
>> +
>> +#endif /* __UK_SCHED__THREAD_INFO_H__ */
>> diff --git a/lib/uksched/include/uk/sched.h
>> b/lib/uksched/include/uk/sched.h
>> index b21d65c..443dbf3 100644
>> --- a/lib/uksched/include/uk/sched.h
>> +++ b/lib/uksched/include/uk/sched.h
>> @@ -59,7 +59,7 @@ int uk_sched_set_default(struct uk_sched *s);
>> typedef void (*uk_sched_yield_func_t)
>> (struct uk_sched *s);
>>
>> -typedef void (*uk_sched_thread_add_func_t)
>> +typedef int (*uk_sched_thread_add_func_t)
>> (struct uk_sched *s, struct uk_thread *t,
>> struct uk_thread_attr *attr);
>> typedef void (*uk_sched_thread_remove_func_t)
>> @@ -106,13 +106,13 @@ static inline void uk_sched_yield(void)
>> s->yield(s);
>> }
>>
>> -static inline void uk_sched_thread_add(struct uk_sched *s,
>> +static inline int uk_sched_thread_add(struct uk_sched *s,
>> struct uk_thread *t, struct uk_thread_attr *attr)
>> {
>> UK_ASSERT(s);
>> UK_ASSERT(t);
>> t->sched = s;
>> - s->thread_add(s, t, attr);
>> + return s->thread_add(s, t, attr);
>> }
>>
>> static inline void uk_sched_thread_remove(struct uk_sched *s,
>> diff --git a/lib/uksched/include/uk/thread.h
>> b/lib/uksched/include/uk/thread.h
>> index 5ff2dd6..d28c458 100644
>> --- a/lib/uksched/include/uk/thread.h
>> +++ b/lib/uksched/include/uk/thread.h
>> @@ -53,6 +53,7 @@ struct uk_thread {
>> uint32_t flags;
>> __snsec wakeup_time;
>> struct uk_sched *sched;
>> + void *sched_info;
>> #ifdef CONFIG_HAVE_LIBC
>> struct _reent reent;
>> #endif
>> diff --git a/lib/uksched/sched.c b/lib/uksched/sched.c
>> index f2eca33..968723c 100644
>> --- a/lib/uksched/sched.c
>> +++ b/lib/uksched/sched.c
>> @@ -196,7 +196,11 @@ struct uk_thread *uk_sched_thread_create(struct
>> uk_sched *sched,
>> if (rc)
>> goto err;
>>
>> - uk_sched_thread_add(sched, thread, attr);
>> + rc = uk_sched_thread_add(sched, thread, attr);
>> + if (rc) {
>> + uk_thread_fini(thread, sched->allocator);
>> + goto err;
>> + }
>>
>> return thread;
>>
>> @@ -213,6 +217,8 @@ void uk_sched_thread_destroy(struct uk_sched *sched,
>> struct uk_thread *thread)
>> {
>> UK_ASSERT(sched != NULL);
>> UK_ASSERT(thread != NULL);
>> +
>> + uk_free(sched->allocator, thread->sched_info);
>> uk_thread_fini(thread, sched->allocator);
>> uk_pfree(sched->allocator, thread->stack, STACK_SIZE_PAGE_ORDER);
>> uk_free(sched->allocator, thread);
>> diff --git a/lib/uksched/thread.c b/lib/uksched/thread.c
>> index a6d90fc..3e69cfe 100644
>> --- a/lib/uksched/thread.c
>> +++ b/lib/uksched/thread.c
>> @@ -34,6 +34,7 @@
>> #include <uk/plat/config.h>
>> #include <uk/plat/time.h>
>> #include <uk/thread.h>
>> +#include <uk/_thread_info.h>
>> #include <uk/sched.h>
>> #include <uk/print.h>
>> #include <uk/assert.h>
>> @@ -86,6 +87,7 @@ int uk_thread_init(struct uk_thread *thread,
>> /* Not runnable, not exited, not sleeping */
>> thread->flags = 0;
>> thread->wakeup_time = 0LL;
>> + thread->sched_info = NULL;
>>
>> #ifdef CONFIG_HAVE_LIBC
>> //TODO _REENT_INIT_PTR(&thread->reent);
>> diff --git a/lib/uksched/thread_info.c b/lib/uksched/thread_info.c
>> new file mode 100644
>> index 0000000..51d47ed
>> --- /dev/null
>> +++ b/lib/uksched/thread_info.c
>> @@ -0,0 +1,79 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in the
>> + * documentation and/or other materials provided with the
>> distribution.
>> + * 3. Neither the name of the copyright holder nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> from
>> + * this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> "AS IS"
>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>> THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> PURPOSE
>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>> CONTRIBUTORS BE
>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> BUSINESS
>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
>> IN
>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>> OTHERWISE)
>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
>> OF THE
>> + * POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>> + */
>> +
>> +#include <uk/_thread_info.h>
>> +#include <uk/print.h>
>> +
>> +
>> +struct thread_info_base *
>> +thread_info_base_create(struct uk_alloc *a,
>> + const uk_thread_attr_t *attr)
>> +{
>> + struct thread_info_base *tib;
>> +
>> + UK_ASSERT(a != NULL);
>> +
>> + tib = uk_malloc(a, sizeof(struct thread_info_base));
>> + if (tib == NULL) {
>> + uk_printd(DLVL_WARN, "Could not allocate thread info.");
>> + return NULL;
>> + }
>> +
>> + thread_info_base_init(tib, attr);
>> +
>> + return tib;
>> +}
>> +
>> +void
>> +thread_info_base_destroy(struct uk_alloc *a,
>> + struct thread_info_base *tib)
>> +{
>> + UK_ASSERT(a != NULL);
>> + UK_ASSERT(tib != NULL);
>> +
>> + uk_free(a, tib);
>> +}
>> +
>> +void
>> +thread_info_base_init(struct thread_info_base *tib,
>> + const uk_thread_attr_t *attr)
>> +{
>> + UK_ASSERT(tib != NULL);
>> +
>> + /* detach state */
>> + if (attr && attr->is_detached)
>> + tib->is_detached = 1;
>> + else
>> + tib->is_detached = 0;
>> +}
>
> Please use macros instead of 1, 0.
As mentioned in previous reply, is_detached is boolean. In v2 I added
comments for the structure members.
>
> Thanks,
>
> — Felipe
>
>> diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
>> index bc02d08..f616330 100644
>> --- a/lib/ukschedcoop/schedcoop.c
>> +++ b/lib/ukschedcoop/schedcoop.c
>> @@ -32,6 +32,7 @@
>> #include <uk/plat/lcpu.h>
>> #include <uk/plat/time.h>
>> #include <uk/sched.h>
>> +#include <uk/_thread_info.h>
>> #include <uk/schedcoop.h>
>>
>> struct schedcoop_private {
>> @@ -132,17 +133,25 @@ static void schedcoop_schedule(struct uk_sched *s)
>> }
>> }
>>
>> -static void schedcoop_thread_add(struct uk_sched *s, struct uk_thread *t,
>> - struct uk_thread_attr *attr __unused)
>> +static int schedcoop_thread_add(struct uk_sched *s, struct uk_thread *t,
>> + struct uk_thread_attr *attr)
>> {
>> unsigned long flags;
>> struct schedcoop_private *prv = s->prv;
>>
>> + t->sched_info = thread_info_base_create(s->allocator, attr);
>> + if (t->sched_info == NULL) {
>> + uk_printd(DLVL_WARN, "Could not create thread info.");
>> + return -1;
>> + }
>> +
>> set_runnable(t);
>>
>> flags = ukplat_lcpu_save_irqf();
>> UK_TAILQ_INSERT_TAIL(&prv->thread_list, t, thread_list);
>> ukplat_lcpu_restore_irqf(flags);
>> +
>> + return 0;
>> }
>>
>> static void schedcoop_thread_remove(struct uk_sched *s, struct uk_thread
>> *t)
>> --
>> 2.11.0
>>
>>
>> _______________________________________________
>> Minios-devel mailing list
>> Minios-devel@xxxxxxxxxxxxxxxxxxxx
>> https://lists.xenproject.org/mailman/listinfo/minios-devel
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |