Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory

[Michal Orzel <michal.orzel@xxxxxxx>]

On 10/04/2024 12:56, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>> On 10 Apr 2024, at 11:01, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>
>> Hi Luca,
>>
>> On 09/04/2024 13:45, Luca Fancellu wrote:
>>>
>>>
>>> Currently the memory footprint of the static shared memory feature
>>> is impacting all the struct meminfo instances with memory space
>>> that is not going to be used.
>>>
>>> To solve this issue, rework the static shared memory extra
>>> information linked to the memory bank to another structure,
>>> struct shmem_membank_extra, and exploit the struct membank
>>> padding to host a pointer to that structure in a union with the
>>> enum membank_type, with this trick the 'struct membank' has the
>>> same size with or without the static shared memory, given that
>>> the 'type' and 'shmem_extra' are never used at the same time,
>>> hence the 'struct membank' won't grow in size.
>>>
>>> Afterwards, create a new structure 'struct shared_meminfo' which
>>> has the same interface of 'struct meminfo', but requires less
>> I would expect some justification for selecting 32 as the max number of 
>> shmem banks
> 
> So I have to say I picked up a value I thought was ok for the amount of 
> shared memory
> Banks, do you think it is too low? The real intention here was to decouple 
> the number
> of shared memory banks from the number of generic memory banks, and I felt 32 
> was enough,
> but if you think it might be an issue I could bump it, or we could have a 
> Kconfig...
No need for Kconfig. 32 is enough for now but I expect a paragraph in commit 
msg that you select
32 which should be enough for current use cases and can be bumped in the future 
in case there is a need.

> 
>>>
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> With the find_unallocated_memory() issue fixed:
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> Thanks, I took the opportunity to improve the comment in that function in 
> this way,
> adding “ (when the feature is enabled)":
> 
>      * 3) Remove static shared memory (when the feature is enabled)
> 
> Please tell me if that works for you so I will keep your R-by
You can retain Rb.

~Michal