[Cbe-oss-dev] [PATCH 02/10]MARS/core: Workload module split make headers public

[Yuji Mano]

Kazunori Asayama wrote:
>> -struct mars_mutex;
>> +struct mars_mutex {
>> +	uint32_t lock;
>> +	uint8_t pad[124];
>> +} __attribute__((aligned(MARS_MUTEX_ALIGN)));
> 
> It seems to be unnecessary to make mars_mutex structure public, since
> workload module implementation doesn't access any member of mars_mutex
> directly.

Yes. However, the current task synchronization structures casts itself to
a mars_mutex and passes it into the mars_mutex APIs...

mars_mutex_lock_get(queue_ea, (struct mars_mutex *)&queue);

This is done because the mars_mutex structure only modifies the first
32-bits of the 128-byte area. Therefore the task synchronization
structures reserve the first 32-bits for the mutex and the rest for its
own use, allowing it to be passed into the mars_mutex APIs.

If we make the mars_mutex structure private, I think the cast would be
really bad (even more so than now) because at that point we would be
casting it blindly to an unknown structure.

It also creates the following compiler warning:
warning: type-punning to incomplete type might break strict-aliasing rules

Do you have any other suggestions?

>> + * MARS workload queue block bits
>> + * ----------------------------------------------------------------------------
>> + * |[63...60]|[59...56]|[55....48]|[  47  ]|[46....32]|[31.....16]|[15......0]|
>> + * ----------------------------------------------------------------------------
>> + * |  4-bits |  4-bits |  8-bits  |  1-bit |  15-bits |  16-bits  |  16-bits  |
>> + * ----------------------------------------------------------------------------
>> + * |   TYPE  |  STATE  | PRIORITY | SIGNAL | RESERVED |  WAIT_ID  |  COUNTER  |
>> + * ----------------------------------------------------------------------------
>> + */
>> +#define MARS_BITS_SIZE				64
>> +
>> +#define MARS_BITS_SHIFT_TYPE			60
>> +#define MARS_BITS_SHIFT_STATE			56
>> +#define MARS_BITS_SHIFT_PRIORITY		48
>> +#define MARS_BITS_SHIFT_SIGNAL			47
>> +#define MARS_BITS_SHIFT_WAIT_ID			16
>> +#define MARS_BITS_SHIFT_COUNTER			0
>> +
>> +#define MARS_BITS_MASK_TYPE			0xf000000000000000ULL
>> +#define MARS_BITS_MASK_STATE			0x0f00000000000000ULL
>> +#define MARS_BITS_MASK_PRIORITY			0x00ff000000000000ULL
>> +#define MARS_BITS_MASK_SIGNAL			0x0000800000000000ULL
>> +#define MARS_BITS_MASK_WAIT_ID			0x00000000ffff0000ULL
>> +#define MARS_BITS_MASK_COUNTER			0x000000000000ffffULL
>> +
>> +#define MARS_BITS_GET(bits, name)  \
>> +	((*(bits) & MARS_BITS_MASK_##name) >> MARS_BITS_SHIFT_##name)
>> +
>> +#define MARS_BITS_SET(bits, name, val) \
>> +	(*bits) = ((*(bits) & ~MARS_BITS_MASK_##name) | \
>> +		((uint64_t)(val) << MARS_BITS_SHIFT_##name))
> 
> Should these definitions be public?

I will rework the module API and keep these definitions private.

Regards,
Yuji