[Cbe-oss-dev] [PATCH 8/10] MARS: workload queue mutex protection

[Geoff Levand]

Yuji Mano wrote:
> Kazunori Asayama wrote:
>> Yuji Mano wrote:
>>> This adds mutex protection when accessing the shared workload queue.
>>> 
>>> Prior to this patch the kernel scheduler accessed the whole queue block
>>> atomically. Now the kernel scheduler atomically locks the queue block when it
>>> needs to access it.
>>> 
>>> Host-side access of the queue block now also uses the mutex locks to keep the
>>> workload queue functions thread-safe.
>>> 
>>> This also replaces the previously used MARS context mutex. The workload queue
>>> handles its own mutex without the workload model APIs needing to manage the
>>> MARS context mutex.
>>> 
>>> Signed-off-by: Yuji Mano <yuji.mano at am.sony.com>
>> 
>> (snip)
>> 
>>> --- a/include/common/mars/mars_workload_types.h
>>> +++ b/include/common/mars/mars_workload_types.h
>>> @@ -64,9 +64,9 @@ extern "C" {
>>>  #define MARS_WORKLOAD_SIGNAL_ON			0x1	/* signal set on */
>>>  #define MARS_WORKLOAD_SIGNAL_OFF		0x0	/* signal set off */
>>>  
>>> -#define MARS_WORKLOAD_MAX			1024	/* wl max */
>>> -#define MARS_WORKLOAD_PER_BLOCK			16	/* wl per block */
>>> -#define MARS_WORKLOAD_NUM_BLOCKS		64	/* wl max / per block */
>>> +#define MARS_WORKLOAD_PER_BLOCK			15	/* wl/block */
>> 
>> This change of MARS_WORKLOAD_PER_BLOCK from 16 to 15 will cause *real*
>> divide operation on MPU when calculating block number and index in the
>> block by ID, i.e.:
>> 
>>   	int block = id / MARS_WORKLOAD_PER_BLOCK;
>>   	int index = id % MARS_WORKLOAD_PER_BLOCK;
>> 
>> I think we should avoid that, if possible.
>> 
>> Other things look ok to me.
> 
> Unfortunately I can't think of a possible solution with the current
> implementation to keep it 16.
> Of course we can reduce MARS_WORKLOAD_PER_BLOCK, but that might be more wasteful
> than beneficial.
> 

>  /* 128 byte workload queue header structure */
>  struct mars_workload_queue_header {
> +	uint32_t lock;
> +	uint32_t count;
>  	uint64_t queue_ea;
>  	uint64_t context_ea;
> -	uint32_t count;
>  	uint8_t flag;
>  } __attribute__((aligned(MARS_WORKLOAD_QUEUE_HEADER_ALIGN)));

It seems like you would only need a single bit to implement a lock.
I guess the EAs will always be aligned, so there will be unused bits
that could maybe use for the lock.

-Geoff