sof: coherent: Don't mark "struct coherent" as packed

[LaurentiuM1234]

On 64-bit systems (i.e: i.MX93), marking "struct coherent" as packed leads to compilation warnings such as:

"warning: taking address of packed member of 'struct coherent' may result in an unaligned pointer value"

This is because the "list" attribute is found at offset 36, which is not 8B-aligned.

Since marking "struct coherent" as packed doesn't seem to be necessary, remove this to fix the compilation warnings.

A more detailed discussion regarding this issue can be found here: #8521.

[kv2019i]

Adding @lyakh and @ranj063 to review. I can't recall why this was packed, but let's check with people working on the code. One part of coherent was that the "struct header" it adds to objects would need to be cache-line aligned. Not sure if packing is the right approach even then.

[lgirdwood]

Packing was done to optimize on cache lines, but the atomic lock needs to be arch specific for size and alignment. i..e removing the pack should be fine on 32bit as types should naturally shrink.

[marc-hb]

You could/should say in the commit message that pahole shows that packed does not make any difference on 32 bits at the moment.

[LaurentiuM1234]

You could/should say in the commit message that pahole shows that packed does not make any difference on 32 bits at the moment.

done!

[LaurentiuM1234]

Packing was done to optimize on cache lines, but the atomic lock needs to be arch specific for size and alignment. i..e removing the pack should be fine on 32bit as types should naturally shrink.

By this you mean that packing struct coherent helps with fitting a structure/multiple structures using it in a single data cache line (since it should take less space)?

[lgirdwood]

Packing was done to optimize on cache lines, but the atomic lock needs to be arch specific for size and alignment. i..e removing the pack should be fine on 32bit as types should naturally shrink.

By this you mean that packing struct coherent helps with fitting a structure/multiple structures using it in a single data cache line (since it should take less space)?

Yes, packing meant we helped optimise any struct fields to be on the same cache line. Never needed in practice though as the struct never grew much and was always smaller than cache line size.

[marc-hb]

Yes, packing meant we helped optimise any struct fields to be on the same cache line.

I submitted an compile-time assert in new #8539 which makes sure for real.

Never needed in practice though as the struct never grew much and was always smaller than cache line size.

At least AMD's Renoir has a DCACHE_LINE_SIZE of 8 bytes which is too small for struct coherent. There may be others, I didn't really check.

[LaurentiuM1234]

At least AMD's Renoir has a DCACHE_LINE_SIZE of 8 bytes which is too small for struct coherent. There may be others, I didn't really check.

Hmm, I guess we could have went with the re-ordering of the struct coherent's attributes such that struct list_item is the first attribute and keeping struct coherent packed but in cases such as this one, keeping the packed attribute won't do much good either.