[PATCH v3 3/3] lib: sbi: Implement aligned memory allocators
Anup Patel
apatel at ventanamicro.com
Thu Aug 8 20:03:02 PDT 2024
On Thu, Aug 8, 2024 at 11:17 PM Gregor Haas <gregorhaas1997 at gmail.com> wrote:
>
> 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.
sbi_aligned_alloc() (suggested by Jessica) is also fine since it
aligns with C11.
>
> 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?
General practice is to send a new version of the entire series. Also, carry
Reviewed-by tags in the commit description after your Signed-off-by in
all the patches which were reviewed so far.
>
> >
> >> +
> >> /** 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
>
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Regards,
Anup
More information about the opensbi
mailing list