[Cbe-oss-dev] [PATCH 10/10 V2] MARS: workload type move from context to queue block

[Geoff Levand]

On 08/06/2008 05:47 PM, Yuji Mano wrote:
> Geoff Levand wrote:
>> 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).
>>> 
>>> Signed-off-by: Yuji Mano <yuji.mano at am.sony.com>
>>> 
>>> ---
>>> Changes since V1:
>>>  - remove unnecessary pad in task context
>>>  - rename pad to context in workload context
>> 
>> No mention of my comments on workload_queue_add() and
>> mars_workload_to_task()...
>> 
>>> @@ -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);
>> 
>> The workload arg to workload_queue_add() will always need to be cast, so
>> is meaningless, and so should just be a void* type.
> 
> Why make it void when the function is expecting a pointer to a mars workload
> context or some derived structure? Even though the caller needs to cast it,
> at least it's more obvious what they need to pass in. I am thinking similar
> to C++ derived classes where struct mars_workload_context is the base class.
> It makes perfect sense in C++ to pass the base class pointer...why not here?

But this is not quite the same.  Here we have a union of the workload context
and a task context.  A C++ derived type is a containment and would be more like

struct mars_task_context {
  struct mars_workload_context base;
  ...
};

Then the base member would be passed.

I think to have more strict typing you could use something like the union
you originally had, but pulled out of the workload context:

union mars_workload_type {
  struct mars_workload_context workload;
  struct mars_task_context task;
};


> 
>>>  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
>> 
>> 
>>> @@ -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);
>> 
>> In general, casts in open code like this are undesirable.  Make a
>> mars_workload_to_task() or some such routine.
> 
> I'm not sure if having an inline function that does the same thing is any
> better. The mars_workload_context is a purely abstract container and should
> never change or need to change. The workload context structure and task
> context structure should be referring to the same thing, the only difference
> being that the task context has member definitions. 

I think my union suggestion could avoid this casting by using the union's
workload variable.

> Is it also undesirable to cast a uint64_t ea to a struct pointer?

I think a routine like from_ea() would be more understandable, and
could help avoid casting problems:

  mars_task.c: In function 'mars_task_initialize':
  mars_task.c:100: warning: cast to pointer from integer of different size
  mars_task.c: In function 'mars_task_finalize':
  mars_task.c:121: warning: cast to pointer from integer of different size
  mars_task.c:134: warning: cast to pointer from integer of different size
  mars_task.c: In function 'mars_task_schedule':
  mars_task.c:152: warning: cast to pointer from integer of different size
  mars_task.c: In function 'mars_task_wait':
  mars_task.c:188: warning: cast to pointer from integer of different size
  mars_task.c: In function 'mars_task_try_wait':
  mars_task.c:203: warning: cast to pointer from integer of different size
  mars_task_event_flag.c:108: warning: cast to pointer from integer of different size
  mars_task_queue.c: In function 'mars_task_queue_clear':
  mars_task_queue.c:113: warning: cast to pointer from integer of different size
  mars_task_queue.c: In function 'push_update':
  mars_task_queue.c:138: warning: cast to pointer from integer of different size

-Geoff