diff mbox

MARS: Remove api argument params structs

Message ID 48CB0D0A.1080302@am.sony.com
State New
Delegated to: Yuji Mano
Headers show

Commit Message

Yuji Mano Sept. 13, 2008, 12:44 a.m. UTC 
This removes the mars_params and mars_task_params structure previously passed
into the mars context and mars task APIs.

Parameter arguments are now passed in directly.

Signed-off-by: Yuji Mano <yuji.mano@am.sony.com>

Comments

Kazunori Asayama Sept. 16, 2008, 6:45 a.m. UTC | #1 
Yuji Mano wrote:
> This removes the mars_params and mars_task_params structure previously passed
> into the mars context and mars task APIs.
> 
> Parameter arguments are now passed in directly.
> 
> Signed-off-by: Yuji Mano <yuji.mano@am.sony.com>
> 
> ---
> v3:
>  - revert v2 change and keep num_mpus_max() function but cache the result of
>    spe_cpu_info_get (so we can use the cached spe count later if needed)
> v2:
>  - replace num_mpus_max() with mpu_check() that avoids calling spe_cpu_info_get
>    multiple times
> 
>  include/host/mars/mars_context.h |   23 +++---------------
>  include/host/mars/mars_task.h    |   32 +++++++------------------
>  src/host/lib/mars_context.c      |   49 +++++++++++++++++----------------------
>  src/host/lib/mars_task.c         |   34 +++++++++++----------------
>  4 files changed, 50 insertions(+), 88 deletions(-)
(snip)
> --- a/src/host/lib/mars_context.c
> +++ b/src/host/lib/mars_context.c
> @@ -48,9 +48,17 @@
>  
>  extern struct spe_program_handle mars_kernel_entry;
>  
> -static int num_mpus_max(void)
> +static uint32_t num_mpus_max(void)
>  {
> -	return spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
> +	static int count = -1;
> +
> +	if (count < 0) {
> +		count = spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
> +		if (count < 0)
> +			count = 0;
> +	}
> +
> +	return count;
>  }

This doesn't seem to be thread-safe?
Yuji Mano Sept. 17, 2008, 7:12 p.m. UTC | #2 
Kazunori Asayama wrote:
> Yuji Mano wrote:
>> This removes the mars_params and mars_task_params structure previously passed
>> into the mars context and mars task APIs.
>> 
>> Parameter arguments are now passed in directly.
>> 
>> Signed-off-by: Yuji Mano <yuji.mano@am.sony.com>
>> 
>> ---
>> v3:
>>  - revert v2 change and keep num_mpus_max() function but cache the result of
>>    spe_cpu_info_get (so we can use the cached spe count later if needed)
>> v2:
>>  - replace num_mpus_max() with mpu_check() that avoids calling spe_cpu_info_get
>>    multiple times
>> 
>>  include/host/mars/mars_context.h |   23 +++---------------
>>  include/host/mars/mars_task.h    |   32 +++++++------------------
>>  src/host/lib/mars_context.c      |   49 +++++++++++++++++----------------------
>>  src/host/lib/mars_task.c         |   34 +++++++++++----------------
>>  4 files changed, 50 insertions(+), 88 deletions(-)
> (snip)
>> --- a/src/host/lib/mars_context.c
>> +++ b/src/host/lib/mars_context.c
>> @@ -48,9 +48,17 @@
>>  
>>  extern struct spe_program_handle mars_kernel_entry;
>>  
>> -static int num_mpus_max(void)
>> +static uint32_t num_mpus_max(void)
>>  {
>> -	return spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
>> +	static int count = -1;
>> +
>> +	if (count < 0) {
>> +		count = spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
>> +		if (count < 0)
>> +			count = 0;
>> +	}
>> +
>> +	return count;
>>  }
> 
> This doesn't seem to be thread-safe?
> 

Yes it is not thread-safe. This fix allows spe_cpu_info_get() to be called only once,
even after we allow for shared contexts and mars_initialize() may be called multiple
times.

I can fix this so that spe_cpu_info_get() gets called once per each mars_initialize()
call, but I think once we have the global shared context we still have the same
problem?

Regards,
Yuji
Yuji Mano Sept. 17, 2008, 7:36 p.m. UTC | #3 
Yuji Mano wrote:
> Kazunori Asayama wrote:
>> Yuji Mano wrote:
>>> This removes the mars_params and mars_task_params structure previously passed
>>> into the mars context and mars task APIs.
>>> 
>>> Parameter arguments are now passed in directly.
>>> 
>>> Signed-off-by: Yuji Mano <yuji.mano@am.sony.com>
>>> 
>>> ---
>>> v3:
>>>  - revert v2 change and keep num_mpus_max() function but cache the result of
>>>    spe_cpu_info_get (so we can use the cached spe count later if needed)
>>> v2:
>>>  - replace num_mpus_max() with mpu_check() that avoids calling spe_cpu_info_get
>>>    multiple times
>>> 
>>>  include/host/mars/mars_context.h |   23 +++---------------
>>>  include/host/mars/mars_task.h    |   32 +++++++------------------
>>>  src/host/lib/mars_context.c      |   49 +++++++++++++++++----------------------
>>>  src/host/lib/mars_task.c         |   34 +++++++++++----------------
>>>  4 files changed, 50 insertions(+), 88 deletions(-)
>> (snip)
>>> --- a/src/host/lib/mars_context.c
>>> +++ b/src/host/lib/mars_context.c
>>> @@ -48,9 +48,17 @@
>>>  
>>>  extern struct spe_program_handle mars_kernel_entry;
>>>  
>>> -static int num_mpus_max(void)
>>> +static uint32_t num_mpus_max(void)
>>>  {
>>> -	return spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
>>> +	static int count = -1;
>>> +
>>> +	if (count < 0) {
>>> +		count = spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
>>> +		if (count < 0)
>>> +			count = 0;
>>> +	}
>>> +
>>> +	return count;
>>>  }
>> 
>> This doesn't seem to be thread-safe?
>> 
> 
> Yes it is not thread-safe. This fix allows spe_cpu_info_get() to be called only once,
> even after we allow for shared contexts and mars_initialize() may be called multiple
> times.
> 
> I can fix this so that spe_cpu_info_get() gets called once per each mars_initialize()
> call, but I think once we have the global shared context we still have the same
> problem?

Sorry. What I meant was I think we can deal with making mars_initialize() thread-safe
once we implement the shared contexts. Or we can protect mars_initialize with a pthread
mutex now.

Regards,
Yuji
Kazunori Asayama Sept. 18, 2008, 2:22 a.m. UTC | #4 
Yuji Mano wrote:
> Yuji Mano wrote:
>> Kazunori Asayama wrote:
>>> Yuji Mano wrote:
>>>> --- a/src/host/lib/mars_context.c
>>>> +++ b/src/host/lib/mars_context.c
>>>> @@ -48,9 +48,17 @@
>>>>  
>>>>  extern struct spe_program_handle mars_kernel_entry;
>>>>  
>>>> -static int num_mpus_max(void)
>>>> +static uint32_t num_mpus_max(void)
>>>>  {
>>>> -	return spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
>>>> +	static int count = -1;
>>>> +
>>>> +	if (count < 0) {
>>>> +		count = spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
>>>> +		if (count < 0)
>>>> +			count = 0;
>>>> +	}
>>>> +
>>>> +	return count;
>>>>  }
>>> This doesn't seem to be thread-safe?
>>>
>> Yes it is not thread-safe. This fix allows spe_cpu_info_get() to be called only once,
>> even after we allow for shared contexts and mars_initialize() may be called multiple
>> times.
>>
>> I can fix this so that spe_cpu_info_get() gets called once per each mars_initialize()
>> call, but I think once we have the global shared context we still have the same
>> problem?
> 
> Sorry. What I meant was I think we can deal with making mars_initialize() thread-safe
> once we implement the shared contexts. Or we can protect mars_initialize with a pthread
> mutex now.

I think an easy solution at this point is to store the return value of
num_mpu_max() in a local auto variable (not a static variable) of
mars_initialize() and reuse it during each mars_initialize() call.
diff mbox

Patch

--- a/include/host/mars/mars_context.h
+++ b/include/host/mars/mars_context.h
@@ -44,32 +44,17 @@ 
  * \brief [host] MARS Context API
  */
 
+#include <stdint.h>
 #include <pthread.h>
 
 /**
  * \ingroup group_mars_context
- * \brief MARS params structure
- *
- * This structure is initialized by the user and passed into
- * \ref mars_initialize for MARS context initialization.
- */
-struct mars_params {
-	/** number of mpus utilized by each MARS context
-	 * [between 0 and maximum available physical MPUs]
-	 */
-	unsigned int num_mpus;
-};
-
-/**
- * \ingroup group_mars_context
  * \brief MARS context structure
  *
  * An instance of this structure must be created and initialized before
  * using any of the MARS API.
  */
 struct mars_context {
-	/* parameters for the MARS context */
-	struct mars_params *params;
 	/* parameters for the MARS kernel */
 	struct mars_kernel_params *kernel_params;
 	/* process queue where process requests are added */
@@ -77,7 +62,7 @@  struct mars_context {
 	/* array of mpu context threads */
 	pthread_t *mpu_context_threads;
 	/* num of mpu context threads */
-	unsigned int mpu_context_count;
+	uint32_t mpu_context_count;
 };
 
 #if defined(__cplusplus)
@@ -98,7 +83,7 @@  extern "C" {
  * each MARS context will suffer the large over head of MPU context switches.
  *
  * \param[out] ctx		- address of context instance to initialize
- * \param[in] params		- pointer to MARS parameters structure
+ * \param[in] num_mpus		- number of mpus utilized by MARS context
  * \return
  *	MARS_SUCCESS		- successfully initialized MARS context
  * \n	MARS_ERROR_NULL		- null pointer specified
@@ -106,7 +91,7 @@  extern "C" {
  * \n	MARS_ERROR_MEMORY	- not enough memory
  * \n	MARS_ERROR_INTERNAL	- some internal error occurred
  */
-int mars_initialize(struct mars_context *ctx, struct mars_params *params);
+int mars_initialize(struct mars_context *ctx, uint32_t num_mpus);
 
 /**
  * \ingroup group_mars_context
--- a/include/host/mars/mars_task.h
+++ b/include/host/mars/mars_task.h
@@ -55,24 +55,6 @@  extern "C" {
 #include "mars/mars_task_signal.h"
 #include "mars/mars_task_types.h"
 
-struct mars_context;
-struct mars_exec_context;
-
-/**
- * \ingroup group_mars_task
- * \brief MARS task params structure
- *
- * This structure is initialized by the user and passed into
- * \ref mars_task_initialize for MARS task initialization.
- */
-struct mars_task_params {
-	/** name of task */
-	const char *name;
-	/** address of MPU program elf image */
-	const void *elf_image;
-	/** size of context save area allocated for task */
-	uint32_t context_save_size;
-};
 
 /**
  * \ingroup group_mars_task
@@ -87,8 +69,7 @@  struct mars_task_params {
  * The task is in the finished state upon creation and may be finalized by
  * \ref mars_task_finalize without ever being scheduled for execution.
  *
- * When initializing a MARS task, you must specify its parameters through the
- * \ref mars_task_params structure:
+ * When initializing a MARS task, you must specify its parameters:
  * \n
  * - \e name: The name is optional, but if specified to other than NULL its
  *	length must not exceed \ref MARS_TASK_NAME_LEN_MAX.
@@ -107,7 +88,9 @@  struct mars_task_params {
  *
  * \param[in] mars		- pointer to initialized MARS context
  * \param[out] id		- pointer to task id to be initialized
- * \param[in] params		- pointer to task params of this task
+ * \param[in] name		- name of task
+ * \param[in] elf_image		- address of MPU program elf image
+ * \param[in] context_save_size - size of context save area allocated for task
  * \return
  *	MARS_SUCCESS		- successfully initialized MARS task
  * \n	MARS_ERROR_NULL		- null pointer specified
@@ -115,8 +98,11 @@  struct mars_task_params {
  * \n	MARS_ERROR_MEMORY	- not enough memory
  * \n	MARS_ERROR_LIMIT	- task queue is currently full
  */
-int mars_task_initialize(struct mars_context *mars, struct mars_task_id *id,
-			struct mars_task_params *params);
+int mars_task_initialize(struct mars_context *mars,
+			struct mars_task_id *id,
+			const char *name,
+			const void *elf_image,
+			uint32_t context_save_size);
 
 /**
  * \ingroup group_mars_task
--- a/src/host/lib/mars_context.c
+++ b/src/host/lib/mars_context.c
@@ -48,9 +48,17 @@ 
 
 extern struct spe_program_handle mars_kernel_entry;
 
-static int num_mpus_max(void)
+static uint32_t num_mpus_max(void)
 {
-	return spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
+	static int count = -1;
+
+	if (count < 0) {
+		count = spe_cpu_info_get(SPE_COUNT_PHYSICAL_SPES, -1);
+		if (count < 0)
+			count = 0;
+	}
+
+	return count;
 }
 
 static void *mpu_context_thread(void *arg)
@@ -77,7 +85,7 @@  static void *mpu_context_thread(void *ar
 	return (void *)MARS_SUCCESS;
 }
 
-static int create_mpu_contexts(struct mars_context *mars, int num_mpu_contexts)
+static int create_mpu_contexts(struct mars_context *mars, uint32_t num_mpus)
 {
 	MARS_ASSERT(mars);
 
@@ -85,7 +93,7 @@  static int create_mpu_contexts(struct ma
 	unsigned int i;
 
 	/* create threads for each mpu context */
-	for (i = 0; i < num_mpu_contexts; i++) {
+	for (i = 0; i < num_mpus; i++) {
 		struct mars_kernel_params *params = &mars->kernel_params[i];
 		params->id = i;
 		params->mars_context_ea =
@@ -120,39 +128,27 @@  static int destroy_mpu_contexts(struct m
 	return MARS_SUCCESS;
 }
 
-int mars_initialize(struct mars_context *mars, struct mars_params *params)
+int mars_initialize(struct mars_context *mars, uint32_t num_mpus)
 {
 	MARS_CHECK_RET(mars, MARS_ERROR_NULL);
+	MARS_CHECK_RET(num_mpus_max() > 0, MARS_ERROR_INTERNAL);
+	MARS_CHECK_RET(num_mpus_max() >= num_mpus, MARS_ERROR_PARAMS);
 
 	MARS_PRINT("Initialize MARS Context (%p)\n", mars);
 
 	int ret;
-	unsigned int num_mpu_contexts;
+
+	/* num mpus specified is default max */
+	if (!num_mpus)
+		num_mpus = num_mpus_max();
 
 	/* zero context */
 	memset(mars, 0, sizeof(struct mars_context));
 
-	/* allocate and initialize params */
-	if (params) {
-		MARS_CHECK_RET(params->num_mpus <= num_mpus_max(),
-				MARS_ERROR_PARAMS);
-
-		mars->params = (struct mars_params *)
-			malloc(sizeof(struct mars_params));
-		MARS_CHECK_RET(mars->params, MARS_ERROR_MEMORY);
-		memcpy(mars->params, params, sizeof(struct mars_params));
-
-		num_mpu_contexts = mars->params->num_mpus;
-	} else {
-		mars->params = NULL;
-
-		num_mpu_contexts = num_mpus_max();
-	}
-
 	/* allocate kernel params */
 	mars->kernel_params = (struct mars_kernel_params *)
 		memalign(MARS_KERNEL_PARAMS_ALIGN,
-			sizeof(struct mars_kernel_params) * num_mpu_contexts);
+			sizeof(struct mars_kernel_params) * num_mpus);
 	MARS_CHECK_CLEANUP_RET(mars->kernel_params, mars_finalize(mars),
 				MARS_ERROR_MEMORY);
 
@@ -169,12 +165,12 @@  int mars_initialize(struct mars_context 
 
 	/* allocate mpu context thread array */
 	mars->mpu_context_threads = (pthread_t *)
-		malloc(sizeof(pthread_t) * num_mpu_contexts);
+		malloc(sizeof(pthread_t) * num_mpus);
 	MARS_CHECK_CLEANUP_RET(mars->mpu_context_threads, mars_finalize(mars),
 				MARS_ERROR_MEMORY);
 
 	/* create contexts */
-	ret = create_mpu_contexts(mars, num_mpu_contexts);
+	ret = create_mpu_contexts(mars, num_mpus);
 	MARS_CHECK_CLEANUP_RET(ret == MARS_SUCCESS, mars_finalize(mars), ret);
 
 	return MARS_SUCCESS;
@@ -199,7 +195,6 @@  int mars_finalize(struct mars_context *m
 	}
 
 	/* free allocated memory */
-	free(mars->params);
 	free(mars->kernel_params);
 	free(mars->workload_queue);
 	free(mars->mpu_context_threads);
--- a/src/host/lib/mars_task.c
+++ b/src/host/lib/mars_task.c
@@ -45,17 +45,23 @@ 
 #include "mars/mars_error.h"
 #include "mars/mars_debug.h"
 
-int mars_task_initialize(struct mars_context *mars, struct mars_task_id *id,
-			struct mars_task_params *params)
+int mars_task_initialize(struct mars_context *mars,
+			struct mars_task_id *id,
+			const char *name,
+			const void *elf_image,
+			uint32_t context_save_size)
 {
 	MARS_CHECK_RET(mars, MARS_ERROR_NULL);
 	MARS_CHECK_RET(id, MARS_ERROR_NULL);
-	MARS_CHECK_RET(params, MARS_ERROR_NULL);
-	MARS_CHECK_RET(params->elf_image, MARS_ERROR_PARAMS);
+	MARS_CHECK_RET(elf_image, MARS_ERROR_NULL);
+	MARS_CHECK_RET(!name || strlen(name) < MARS_TASK_NAME_LEN_MAX,
+			MARS_ERROR_PARAMS);
+	MARS_CHECK_RET(context_save_size <= MARS_TASK_CONTEXT_SAVE_SIZE_MAX,
+			MARS_ERROR_PARAMS);
 
 	int ret;
 	struct mars_task_context *task;
-	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)params->elf_image;
+	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)elf_image;
 	Elf32_Phdr *phdr = (Elf32_Phdr *)((void *)ehdr + ehdr->e_phoff);
 
 	//MARS_CHECK_RET(ehdr->e_phnum == 1, MARS_ERROR_FORMAT);
@@ -67,11 +73,8 @@  int mars_task_initialize(struct mars_con
 	id->mars_context_ea = (uint64_t)(uintptr_t)mars;
 
 	/* 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 *)&id->name, params->name);
-	}
+	if (name)
+		strcpy((char *)&id->name, name);
 
 	/* begin process to add the task to the workload queue */
 	ret = workload_queue_add_begin(mars->workload_queue, &id->workload_id,
@@ -90,15 +93,8 @@  int mars_task_initialize(struct mars_con
 	task->entry = ehdr->e_entry;
 
 	/* allocate the task context area if specified */
-	if (params->context_save_size) {
-		MARS_CHECK_CLEANUP_RET(params->context_save_size <=
-			MARS_TASK_CONTEXT_SAVE_SIZE_MAX,
-			workload_queue_add_cancel(mars->workload_queue,
-				id->workload_id),
-			MARS_ERROR_PARAMS);
-
-		task->context_save_size =
-			params->context_save_size;
+	if (context_save_size) {
+		task->context_save_size = context_save_size;
 		task->context_save_area = (uint64_t)(uintptr_t)
 			memalign(MARS_TASK_CONTEXT_SAVE_ALIGN,
 				task->context_save_size);