[Cbe-oss-dev] [PATCH 10/10] MARS: workload type move from context to queue block
Geoff Levand
geoffrey.levand at am.sony.com
Wed Aug 6 10:59:39 EST 2008
Yuji Mano wrote:
> Make the workload context a completely abstract container.
> Previously the workload type was part of the workload context, but this moves
> the type into the inherited workload context (task context).
Running this through checkpatch gives me this:
WARNING: externs should be avoided in .c files
#298: FILE: src/mpu/kernel/mars_kernel_workload.c:43:
+extern uint8_t workload_type;
ERROR: space required before the open parenthesis '('
#307: FILE: src/mpu/kernel/mars_kernel_workload.c:69:
+ switch(workload_type) {
ERROR: space required before the open parenthesis '('
#319: FILE: src/mpu/kernel/mars_kernel_workload.c:82:
+ switch(workload_type) {
ERROR: space required before the open parenthesis '('
#331: FILE: src/mpu/kernel/mars_kernel_workload.c:95:
+ switch(workload_type) {
ERROR: space required before the open parenthesis '('
#347: FILE: src/mpu/kernel/mars_kernel_workload.c:112:
+ switch(workload_type) {
ERROR: space required before the open parenthesis '('
#378: FILE: src/mpu/kernel/mars_kernel_workload.c:164:
+ switch(schedule_workload_type) {
ERROR: space required before the open parenthesis '('
#391: FILE: src/mpu/kernel/mars_kernel_workload.c:315:
+ switch(workload_type) {
total: 6 errors, 1 warnings, 476 lines checked
> Signed-off-by: Yuji Mano <yuji.mano at am.sony.com>
>
> ---
> include/common/mars/mars_task_types.h | 1
> include/common/mars/mars_workload_types.h | 18 ++++-----
> include/host/mars/mars_workload_queue.h | 2 -
> include/mpu/mars/mars_kernel.h | 1
> src/host/lib/mars_task.c | 56 +++++++++++++-----------------
> src/host/lib/mars_workload_queue.c | 7 ++-
> src/mpu/kernel/mars_kernel_scheduler.c | 6 +--
> src/mpu/kernel/mars_kernel_workload.c | 30 +++++++++-------
> src/mpu/lib/mars_task.c | 21 ++++++++---
> src/mpu/lib/mars_task_barrier.c | 5 +-
> src/mpu/lib/mars_task_event_flag.c | 5 +-
> src/mpu/lib/mars_task_queue.c | 5 +-
> src/mpu/lib/mars_task_semaphore.c | 5 +-
> src/mpu/lib/mars_task_signal.c | 5 +-
> 14 files changed, 90 insertions(+), 77 deletions(-)
>
> --- a/include/common/mars/mars_task_types.h
> +++ b/include/common/mars/mars_task_types.h
> @@ -156,6 +156,7 @@ struct mars_task_context {
> uint32_t stack; /* stack pointer of exec */
> uint32_t context_save_size; /* context save size */
> uint64_t context_save_area; /* context save area */
> + uint8_t pad[24]; /* padding */
> } __attribute__((aligned(MARS_TASK_CONTEXT_ALIGN)));
It doesn't seem you need to add this pad variable.
>
> #if defined(__cplusplus)
> --- a/include/common/mars/mars_workload_types.h
> +++ b/include/common/mars/mars_workload_types.h
> @@ -43,18 +43,17 @@ extern "C" {
> #endif
>
> #include <stdint.h>
> -#include "mars/mars_task_types.h"
> -
> -#define MARS_WORKLOAD_TYPE_UNKNOWN 0x0 /* workload undefined */
> -#define MARS_WORKLOAD_TYPE_TASK 0x1 /* workload type task */
>
> #define MARS_WORKLOAD_ID_NONE 0xffff /* workload id none */
Seems -1 would be better here.
> +#define MARS_WORKLOAD_TYPE_NONE 0x00 /* workload undefined */
> +#define MARS_WORKLOAD_TYPE_TASK 0x01 /* workload type */
> +
> #define MARS_WORKLOAD_STATE_NONE 0x00 /* workload undefined */
> #define MARS_WORKLOAD_STATE_READY 0x01 /* ready to schedule */
> #define MARS_WORKLOAD_STATE_WAITING 0x02 /* waiting for sync */
> -#define MARS_WORKLOAD_STATE_RUNNING 0x04 /* currently running */
> -#define MARS_WORKLOAD_STATE_FINISHED 0x08 /* not allow schedule */
> +#define MARS_WORKLOAD_STATE_RUNNING 0x03 /* currently running */
> +#define MARS_WORKLOAD_STATE_FINISHED 0x04 /* not allow schedule */
These should be enums.
> #define MARS_WORKLOAD_PRIORITY_MIN 0x00 /* minimum priority */
> #define MARS_WORKLOAD_PRIORITY_MAX 0xff /* maximum priority */
> @@ -69,6 +68,7 @@ extern "C" {
> #define MARS_WORKLOAD_PER_BLOCK 16 /* wl per block */
> #define MARS_WORKLOAD_NUM_BLOCKS 64 /* wl max / per block */
>
> +#define MARS_WORKLOAD_CONTEXT_SIZE 128 /* size of 128 bytes */
> #define MARS_WORKLOAD_CONTEXT_ALIGN 128 /* align to 128 bytes */
> #define MARS_WORKLOAD_QUEUE_HEADER_ALIGN 128 /* align to 128 bytes */
> #define MARS_WORKLOAD_QUEUE_BLOCK_ALIGN 128 /* align to 128 bytes */
Same here, all enums. Debugger users will be happy then.
> @@ -76,10 +76,7 @@ extern "C" {
>
> /* mars workload context */
> struct mars_workload_context {
> - uint8_t type;
> - union {
> - struct mars_task_context task;
> - } type_context;
> + uint8_t pad[MARS_WORKLOAD_CONTEXT_SIZE];
Seems pad is not a good variable name here, it is fill, not padding.
What about 'context'.
> } __attribute__((aligned(MARS_WORKLOAD_CONTEXT_ALIGN)));
>
> /* 128 byte workload queue header structure */
> @@ -92,6 +89,7 @@ struct mars_workload_queue_header {
>
> /* 128 byte workload queue block structure */
> struct mars_workload_queue_block {
> + uint8_t type[MARS_WORKLOAD_PER_BLOCK];
> uint8_t state[MARS_WORKLOAD_PER_BLOCK];
> uint8_t priority[MARS_WORKLOAD_PER_BLOCK];
> uint16_t counter[MARS_WORKLOAD_PER_BLOCK];
> --- a/include/host/mars/mars_workload_queue.h
> +++ b/include/host/mars/mars_workload_queue.h
> @@ -47,7 +47,7 @@ extern "C" {
> int workload_queue_initialize(struct mars_workload_queue *queue);
> int workload_queue_finalize(struct mars_workload_queue *queue);
> int workload_queue_add(struct mars_workload_queue *queue, uint16_t *id,
> - struct mars_workload_context *workload);
> + struct mars_workload_context *workload, uint8_t type);
> int workload_queue_remove(struct mars_workload_queue *queue, uint16_t id,
> struct mars_workload_context *workload);
> int workload_queue_context(struct mars_workload_queue *queue, uint16_t id,
> --- a/include/mpu/mars/mars_kernel.h
> +++ b/include/mpu/mars/mars_kernel.h
> @@ -49,6 +49,7 @@ extern "C" {
> #include "mars/mars_error.h"
> #include "mars/mars_kernel_types.h"
> #include "mars/mars_workload_types.h"
> +#include "mars/mars_task_types.h"
>
> #define MARS_KERNEL_STATUS_BUSY 0x0
> #define MARS_KERNEL_STATUS_IDLE 0x1
> --- a/src/host/lib/mars_task.c
> +++ b/src/host/lib/mars_task.c
> @@ -54,22 +54,21 @@ int mars_task_initialize(struct mars_con
> MARS_CHECK_RET(params, MARS_ERROR_NULL);
>
> int ret;
> - struct mars_workload_context workload;
> - struct mars_task_context *task = &workload.type_context.task;
> + struct mars_task_context task;
>
> /* initialize task context */
> - task->id.mars_context_ea = (uint64_t)(uintptr_t)mars;
> + task.id.mars_context_ea = (uint64_t)(uintptr_t)mars;
>
> /* parse the elf parameter */
> - ret = mars_elf_parse(params->elf_image, &task->exec, &task->exec_size,
> - &task->bss_size, &task->vaddr, &task->entry);
> + ret = mars_elf_parse(params->elf_image, &task.exec, &task.exec_size,
> + &task.bss_size, &task.vaddr, &task.entry);
> MARS_CHECK_RET(ret == MARS_SUCCESS, ret);
>
> /* copy the task name into task id */
> if (params->name) {
> MARS_CHECK_RET(strlen(params->name) < MARS_TASK_NAME_LEN_MAX,
> MARS_ERROR_PARAMS);
> - strcpy((char *)&task->id.name, params->name);
> + strcpy((char *)&task.id.name, params->name);
What if params->name is too big, should this be strncpy?
> }
>
> /* allocate the task context area if specified */
> @@ -77,32 +76,30 @@ int mars_task_initialize(struct mars_con
> MARS_CHECK_RET(params->context_save_size <=
> MARS_TASK_CONTEXT_SAVE_SIZE_MAX, MARS_ERROR_PARAMS);
>
> - task->context_save_size =
> + task.context_save_size =
> params->context_save_size;
> - task->context_save_area = (uint64_t)(uintptr_t)
> + task.context_save_area = (uint64_t)(uintptr_t)
> memalign(MARS_TASK_CONTEXT_SAVE_ALIGN,
> - task->context_save_size);
> + task.context_save_size);
>
> - MARS_CHECK_RET(task->context_save_area, MARS_ERROR_MEMORY);
> + MARS_CHECK_RET(task.context_save_area, MARS_ERROR_MEMORY);
> } else {
> - task->context_save_size = 0;
> - task->context_save_area = 0;
> + task.context_save_size = 0;
> + task.context_save_area = 0;
> }
>
> mars_mutex_lock(mars->mutex);
>
> - /* set the workload context type to type task */
> - workload.type = MARS_WORKLOAD_TYPE_TASK;
> -
> - /* add the workload to the workload queue */
> - ret = workload_queue_add(mars->workload_queue, &task->id.workload_id,
> - &workload);
> + /* add the task to the workload queue */
> + ret = workload_queue_add(mars->workload_queue, &task.id.workload_id,
> + (struct mars_workload_context *)&task,
workload_queue_add() should change to just take a void* type, then this kind of
senseless cast is not needed.
> + MARS_WORKLOAD_TYPE_TASK);
> MARS_CHECK_CLEANUP_RET(ret == MARS_SUCCESS,
> - free((void *)task->context_save_area), ret);
> - MARS_PRINT_WORKLOAD_CONTEXT(&workload);
> + free((void *)task.context_save_area), ret);
> + MARS_PRINT_TASK_CONTEXT(&task);
>
> /* copy the task id into return id */
> - *id = task->id;
> + *id = task.id;
>
> mars_mutex_unlock(mars->mutex);
>
> @@ -119,23 +116,22 @@ int mars_task_finalize(struct mars_task_
>
> int ret;
> struct mars_context *mars = (struct mars_context *)id->mars_context_ea;
> - struct mars_workload_context workload;
> - struct mars_task_context *task = &workload.type_context.task;
> + struct mars_task_context task;
>
> mars_mutex_lock(mars->mutex);
>
> - /* remove the workload from the workload queue */
> + /* remove the task from the workload queue */
> ret = workload_queue_remove(mars->workload_queue, id->workload_id,
> - &workload);
> + (struct mars_workload_context *)&task);
Same here.
> MARS_CHECK_RET(ret == MARS_SUCCESS, ret);
>
> /* free the allocated task context area if it has one */
> - if (task->context_save_size)
> - free((void *)task->context_save_area);
> + if (task.context_save_size)
> + free((void *)task.context_save_area);
>
> mars_mutex_unlock(mars->mutex);
>
> - MARS_PRINT("Finalize Task Context %d\n", task->id.workload_id);
> + MARS_PRINT("Finalize Task Context %d\n", task.id.workload_id);
>
> return MARS_SUCCESS;
> }
> @@ -150,18 +146,16 @@ int mars_task_schedule(struct mars_task_
>
> int ret;
> struct mars_context *mars = (struct mars_context *)id->mars_context_ea;
> - struct mars_workload_context *workload = NULL;
> struct mars_task_context *task = NULL;
>
> mars_mutex_lock(mars->mutex);
>
> /* get workload context pointer from the workload queue */
> ret = workload_queue_context(mars->workload_queue, id->workload_id,
> - &workload);
> + (struct mars_workload_context **)&task);
And here...
> MARS_CHECK_RET(ret == MARS_SUCCESS, ret);
>
> /* initialize task specific context variables */
> - task = &workload->type_context.task;
> task->stack = 0;
> if (args)
> memcpy(&task->args, args, sizeof(struct mars_task_args));
> --- a/src/host/lib/mars_workload_queue.c
> +++ b/src/host/lib/mars_workload_queue.c
> @@ -69,13 +69,12 @@ int workload_queue_finalize(struct mars_
> }
>
> int workload_queue_add(struct mars_workload_queue *queue, uint16_t *id,
> - struct mars_workload_context *workload)
> + struct mars_workload_context *workload, uint8_t type)
> {
> MARS_CHECK_RET(queue, MARS_ERROR_NULL);
> MARS_CHECK_RET(workload, MARS_ERROR_NULL);
> MARS_CHECK_RET(id, MARS_ERROR_NULL);
> - MARS_CHECK_RET(workload->type == MARS_WORKLOAD_TYPE_TASK,
> - MARS_ERROR_PARAMS);
> + MARS_CHECK_RET(type == MARS_WORKLOAD_TYPE_TASK, MARS_ERROR_PARAMS);
> MARS_CHECK_RET(queue->header.count < MARS_WORKLOAD_MAX,
> MARS_ERROR_LIMIT);
>
> @@ -108,6 +107,7 @@ int workload_queue_add(struct mars_workl
> sizeof(struct mars_workload_context));
>
> /* update workload queue header info */
> + queue->block[block].type[index] = type;
> queue->block[block].state[index] = MARS_WORKLOAD_STATE_FINISHED;
> queue->block[block].priority[index] = MARS_WORKLOAD_PRIORITY_MIN;
> queue->block[block].counter[index] = MARS_WORKLOAD_COUNTER_MIN;
> @@ -136,6 +136,7 @@ int workload_queue_remove(struct mars_wo
> sizeof(struct mars_workload_context));
>
> /* update workload queue info */
> + queue->block[block].type[index] = MARS_WORKLOAD_TYPE_NONE;
> queue->block[block].state[index] = MARS_WORKLOAD_STATE_NONE;
> queue->block[block].priority[index] = MARS_WORKLOAD_PRIORITY_MIN;
> queue->block[block].counter[index] = MARS_WORKLOAD_COUNTER_MIN;
> --- a/src/mpu/kernel/mars_kernel_scheduler.c
> +++ b/src/mpu/kernel/mars_kernel_scheduler.c
> @@ -43,6 +43,7 @@
> struct mars_workload_queue_header queue_header;
> struct mars_workload_queue_block queue_block;
> struct mars_workload_context workload;
> +uint8_t workload_type;
> uint8_t workload_state;
> uint16_t workload_index;
> uint64_t workload_ea;
> @@ -198,10 +199,9 @@ int reserve_workload(void)
> if (index < 0)
> return MARS_WORKLOAD_RESERVED_NONE;
>
> - /* set workload index based on workload block and index */
> + /* set global workload info based on workload block and index */
> workload_index = MARS_WORKLOAD_PER_BLOCK * block + index;
> -
> - /* calculate context ea from workload index */
> + workload_type = queue_block.type[index];
> workload_ea = queue_header.context_ea +
> workload_index * sizeof(struct mars_workload_context);
>
> --- a/src/mpu/kernel/mars_kernel_workload.c
> +++ b/src/mpu/kernel/mars_kernel_workload.c
> @@ -40,6 +40,7 @@
> extern struct mars_workload_queue_header queue_header;
> extern struct mars_workload_queue_block queue_block;
> extern struct mars_workload_context workload;
> +extern uint8_t workload_type;
> extern uint8_t workload_state;
> extern uint16_t workload_index;
> extern uint64_t workload_ea;
> @@ -65,9 +66,9 @@ void workload_run(void)
> workload_state = MARS_WORKLOAD_STATE_RUNNING;
>
> /* workload type specific handling */
> - switch(workload.type) {
> + switch(workload_type) {
> case MARS_WORKLOAD_TYPE_TASK:
> - task_run(&workload.type_context.task);
> + task_run((struct mars_task_context *)&workload);
task_run() should take a mars_workload_context. Then do the cast
to mars_task_context in it, or better, have a routine like
workload_to_task() that does the cast.
> break;
> }
> }
> @@ -78,9 +79,9 @@ void workload_stop(void)
> workload_state = MARS_WORKLOAD_STATE_FINISHED;
>
> /* workload type specific handling */
> - switch(workload.type) {
> + switch(workload_type) {
> case MARS_WORKLOAD_TYPE_TASK:
> - task_stop(&workload.type_context.task);
> + task_stop((struct mars_task_context *)&workload);
> break;
> }
> }
> @@ -91,13 +92,13 @@ void workload_save(void)
> workload_state = MARS_WORKLOAD_STATE_READY;
>
> /* workload type specific handling */
> - switch(workload.type) {
> + switch(workload_type) {
> case MARS_WORKLOAD_TYPE_TASK:
> /* save non-volatile registers */
> registers_save();
>
> /* save the task context to main memory */
> - task_save(&workload.type_context.task);
> + task_save((struct mars_task_context *)&workload);
> break;
> }
> }
> @@ -108,10 +109,10 @@ void workload_restore(void)
> workload_state = MARS_WORKLOAD_STATE_RUNNING;
>
> /* workload type specific handling */
> - switch(workload.type) {
> + switch(workload_type) {
> case MARS_WORKLOAD_TYPE_TASK:
> /* restore the task context from main memory */
> - task_restore(&workload.type_context.task);
> + task_restore((struct mars_task_context *)&workload);
>
> /* restore non-volatile registers */
> registers_restore();
> @@ -125,6 +126,7 @@ int workload_schedule(uint16_t workload_
> MARS_CHECK_RET(workload_id != workload_index, MARS_ERROR_PARAMS);
>
> struct mars_workload_context schedule_workload;
> + uint8_t schedule_workload_type;
> uint64_t schedule_workload_ea;
>
> int block = workload_id / MARS_WORKLOAD_PER_BLOCK;
> @@ -147,7 +149,8 @@ int workload_schedule(uint16_t workload_
> return MARS_ERROR_STATE;
> }
>
> - /* calculate context ea from workload id */
> + /* get information of workload to schedule */
> + schedule_workload_type = queue_block.type[index];
> schedule_workload_ea = queue_header.context_ea +
> workload_id * sizeof(struct mars_workload_context);
>
> @@ -158,9 +161,10 @@ int workload_schedule(uint16_t workload_
> MARS_DMA_TAG);
>
> /* workload type specific handling */
> - switch(schedule_workload.type) {
> + switch(schedule_workload_type) {
> case MARS_WORKLOAD_TYPE_TASK:
> - task_schedule(&schedule_workload.type_context.task,
> + task_schedule((struct mars_task_context *)
> + &schedule_workload,
> (struct mars_task_args *)args);
> break;
> }
> @@ -308,13 +312,13 @@ void workload_signal_wait(void)
> workload_state = MARS_WORKLOAD_STATE_WAITING;
>
> /* workload type specific handling */
> - switch(workload.type) {
> + switch(workload_type) {
> case MARS_WORKLOAD_TYPE_TASK:
> /* save non-volatile registers */
> registers_save();
>
> /* save the task context to main memory */
> - task_save(&workload.type_context.task);
> + task_save((struct mars_task_context *)&workload);
> break;
> }
> }
> --- a/src/mpu/lib/mars_task.c
> +++ b/src/mpu/lib/mars_task.c
> @@ -36,7 +36,7 @@
> */
>
> #include "mars/mars_task.h"
> -#include "mars/mars_workload_types.h"
> +#include "mars/mars_task_types.h"
> #include "mars/mars_syscalls.h"
> #include "mars/mars_error.h"
> #include "mars/mars_debug.h"
> @@ -50,8 +50,9 @@ int mars_task_yield(void)
> {
> struct mars_task_context *task;
>
> + task = (struct mars_task_context *)mars_get_workload_context();
> +
> /* make sure task context has a context save area */
> - task = &mars_get_workload_context()->type_context.task;
> MARS_CHECK_RET(task->context_save_size && task->context_save_area,
> MARS_ERROR_FORMAT);
>
> @@ -76,8 +77,9 @@ int mars_task_wait(struct mars_task_id *
> int ret;
> struct mars_task_context *task;
>
> + task = (struct mars_task_context *)mars_get_workload_context();
Hey, what about a routine mars_workload_to_task() here!
The implementation of struct mars_workload_context
is then hidden.
More information about the cbe-oss-dev
mailing list