[PATCH v3 3/3] lib: sbi: Implement aligned memory allocators
Gregor Haas
gregorhaas1997 at gmail.com
Thu Aug 8 10:47:15 PDT 2024
Hi Anup,
> On Aug 7, 2024, at 10:33 PM, Anup Patel <anup at brainfault.org> wrote:
>
> On Wed, Aug 7, 2024 at 11:47 PM Gregor Haas <gregorhaas1997 at gmail.com> wrote:
>>
>> This change adds a simple implementation of sbi_memalign(), for future use in
>> allocating aligned memory for SMMTT tables.
>>
>> Signed-off-by: Gregor Haas <gregorhaas1997 at gmail.com>
>> ---
>> include/sbi/sbi_heap.h | 9 +++++
>> lib/sbi/sbi_heap.c | 81 ++++++++++++++++++++++++++++++++++++++----
>> 2 files changed, 84 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/sbi/sbi_heap.h b/include/sbi/sbi_heap.h
>> index 9a67090..2103aef 100644
>> --- a/include/sbi/sbi_heap.h
>> +++ b/include/sbi/sbi_heap.h
>> @@ -31,6 +31,15 @@ static inline void *sbi_malloc(size_t size)
>> return sbi_malloc_from(&global_hpctrl, size);
>> }
>>
>> +/** Allocate aligned from heap area */
>> +void *sbi_memalign_from(struct sbi_heap_control *hpctrl, size_t alignment,
>> + size_t size);
>> +
>> +static inline void *sbi_memalign(size_t alignment, size_t size)
>> +{
>> + return sbi_memalign_from(&global_hpctrl, alignment, size);
>> +}
>
> The term "memalign" does not imply it is a memory allocation function.
>
> I suggest the following names instead:
> sbi_malloc_aligned_from()
> sbi_malloc_aligned()
I took this function prototype from POSIX’s memalign, which is a standard allocation
function. I can definitely rename this, but thought I would give my justification.
For all other comments below, agreed — I can integrate these changes. Do you want
me to send a whole new v4 patch series (including the two earlier commits you’ve
reviewed already)? Or send a new version of just this commit?
>
>> +
>> /** Zero allocate from heap area */
>> void *sbi_zalloc_from(struct sbi_heap_control *hpctrl, size_t size);
>>
>> diff --git a/lib/sbi/sbi_heap.c b/lib/sbi/sbi_heap.c
>> index cc4893d..ea73b54 100644
>> --- a/lib/sbi/sbi_heap.c
>> +++ b/lib/sbi/sbi_heap.c
>> @@ -37,27 +37,70 @@ struct sbi_heap_control {
>>
>> struct sbi_heap_control global_hpctrl;
>>
>> -void *sbi_malloc_from(struct sbi_heap_control *hpctrl, size_t size)
>> +static void *alloc_with_align(struct sbi_heap_control *hpctrl,
>> + size_t align, size_t size)
>> {
>> void *ret = NULL;
>> - struct heap_node *n, *np;
>> + struct heap_node *n, *np, *rem;
>> + uint64_t lowest_aligned;
>
> Since this represents the lowest aligned address, the data type
> should be "unsigned long".
>
>> + size_t pad;
>>
>> if (!size)
>> return NULL;
>>
>> - size += HEAP_ALLOC_ALIGN - 1;
>> - size &= ~((unsigned long)HEAP_ALLOC_ALIGN - 1);
>> + size += align - 1;
>> + size &= ~((unsigned long)align - 1);
>>
>> spin_lock(&hpctrl->lock);
>>
>> np = NULL;
>> sbi_list_for_each_entry(n, &hpctrl->free_space_list, head) {
>> - if (size <= n->size) {
>> + lowest_aligned = ROUNDUP(n->addr, align);
>> + pad = lowest_aligned - n->addr;
>> +
>> + if (size + pad <= n->size) {
>> np = n;
>> break;
>> }
>> }
>> - if (np) {
>> + if (!np) {
>> + goto out;
>> + }
>
> No need for {} here.
>
>> +
>> + if (pad) {
>> + if (sbi_list_empty(&hpctrl->free_node_list)) {
>> + goto out;
>> + }
>
> No need for {} here.
>
>> +
>> + n = sbi_list_first_entry(&hpctrl->free_node_list,
>> + struct heap_node, head);
>> + sbi_list_del(&n->head);
>> +
>> + if ((size + pad < np->size) &&
>> + !sbi_list_empty(&hpctrl->free_node_list)) {
>> + rem = sbi_list_first_entry(&hpctrl->free_node_list,
>> + struct heap_node, head);
>> + sbi_list_del(&rem->head);
>> + rem->addr = np->addr + (size + pad);
>> + rem->size = np->size - (size + pad);
>> + sbi_list_add_tail(&rem->head,
>> + &hpctrl->free_space_list);
>> +
>> + n->addr = lowest_aligned;
>> + n->size = size;
>> + np->size = pad;
>> + sbi_list_add_tail(&n->head, &hpctrl->used_space_list);
>> + ret = (void *)n->addr;
>> + } else if (size + pad == np->size) {
>> + n->addr = lowest_aligned;
>> + n->size = size;
>> + np->size = pad;
>> + ret = (void *)n->addr;
>> + } else {
>> + // Can't allocate, return n
>
> Use C-style comments.
>
>> + sbi_list_add(&n->head, &hpctrl->free_node_list);
>> + }
>> + } else {
>> if ((size < np->size) &&
>> !sbi_list_empty(&hpctrl->free_node_list)) {
>> n = sbi_list_first_entry(&hpctrl->free_node_list,
>> @@ -76,11 +119,37 @@ void *sbi_malloc_from(struct sbi_heap_control *hpctrl, size_t size)
>> }
>> }
>>
>> +out:
>> spin_unlock(&hpctrl->lock);
>>
>> return ret;
>> }
>>
>> +void *sbi_malloc_from(struct sbi_heap_control *hpctrl, size_t size)
>> +{
>> + return alloc_with_align(hpctrl, HEAP_ALLOC_ALIGN, size);
>> +}
>> +
>> +void *sbi_memalign_from(struct sbi_heap_control *hpctrl, size_t alignment,
>> + size_t size)
>> +{
>> + if(alignment < HEAP_ALLOC_ALIGN) {
>
> Need a space between "if" and "(".
>
>> + alignment = HEAP_ALLOC_ALIGN;
>> + }
>
> No need for {} here.
>
>> +
>> + // Make sure alignment is power of two
>
> Use C-style comments.
>
>> + if((alignment & (alignment - 1)) != 0) {
>
> Need a space between "if" and "(".
>
>> + return NULL;
>> + }
>
> No need for {} here.
>
>> +
>> + // Make sure size is multiple of alignment
>
> Use C-style comments.
>
>> + if(size % alignment != 0) {
>> + return NULL;
>> + }
>
> No need for {} here.
>
>> +
>> + return alloc_with_align(hpctrl, alignment, size);
>> +}
>> +
>> void *sbi_zalloc_from(struct sbi_heap_control *hpctrl, size_t size)
>> {
>> void *ret = sbi_malloc_from(hpctrl, size);
>> --
>> 2.45.2
>>
>
> Regards,
> Anup
More information about the opensbi
mailing list