Re: [Minios-devel] [UNIKRAFT PATCH 1/2] lib/ukallocbbuddy: Fix definition and assertions of memr.nr_pages

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Bruno,

First of all thanks for pointing out this issue! Please see my comments
inline.

On 03/15/2018 05:38 PM, Bruno Alvisio wrote:
> Currently, nr_pages is set to the range size instead of the number of pages in
> the memory region. Fixed by shifting by __PAGE_SIZE. Assertions are fixed
> accordingly.
> 
> Signed-off-by: Bruno Alvisio <bruno.alvisio@xxxxxxxxx>
> ---
>  lib/ukallocbbuddy/bbuddy.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ukallocbbuddy/bbuddy.c b/lib/ukallocbbuddy/bbuddy.c
> index b830995..13cb0c8 100644
> --- a/lib/ukallocbbuddy/bbuddy.c
> +++ b/lib/ukallocbbuddy/bbuddy.c
> @@ -107,7 +107,7 @@ static inline struct uk_bbpalloc_memr 
> *map_get_memr(struct uk_bbpalloc *b,
>        */
>       for (memr = b->memr_head; memr != NULL; memr = memr->next) {
>               if ((page_num >= memr->first_page)
> -                 && (page_num < (memr->first_page + memr->nr_pages)))
> +                 && (page_num < (memr->first_page + memr->nr_pages << 
> __PAGE_SIZE)))

Here page_num should be renamed to either page or page_va (page virtual
address) because it checks if the page address is in the memory region.
Also if you're using bit shifting then it's __PAGE_SHIFT, not __PAGE_SIZE.

>                       return memr;
>       }
>  
> @@ -145,7 +145,7 @@ static void map_alloc(struct uk_bbpalloc *b, uintptr_t 
> first_page,
>       memr = map_get_memr(b, first_page);
>       UK_ASSERT(memr != NULL);
>       UK_ASSERT((first_page + nr_pages)
> -               <= (memr->first_page + memr->nr_pages));
> +               <= (memr->first_page + memr->nr_pages << __PAGE_SIZE));

Again, it's __PAGE_SHIFT, not __PAGE_SIZE. More than that, the assertion
is incomplete. The correct assertion would be:
+       UK_ASSERT((first_page + (nr_pages << __PAGE_SHIFT))
+             <= (memr->first_page + (memr->nr_pages << __PAGE_SHIFT)));

>  
>       first_page -= memr->first_page;
>       curr_idx = first_page / PAGES_PER_MAPWORD;
> @@ -362,7 +362,8 @@ static int bbuddy_addmem(struct uk_alloc *a, void *base, 
> size_t len)
>        * Initialize region's bitmap
>        */
>       memr->first_page = min;
> -     memr->nr_pages = max - min;
> +     memr->nr_pages = (max - min) >> __PAGE_SIZE;
> +

Again, it's __PAGE_SHIFT, not __PAGE_SIZE.

The number of pages is not the best one. If
range = 8 * PAGE_SIZE * (PAGE_SIZE - sizeof(*memr) + 1), then
we would lose one page for book keeping because
memr_size = round_pgup(PAGE_SIZE + 1).

The right number of page is given by the inequality:
sizeof(*memr) + bitmap_size + page_num * page_size <= range,
where bitmap_size = page_num / BITS_PER_BYTE (please define this macro
in bbuddy.c to avoid confusion). Therefore, the number of pages is:

BITS_PER_BYTE * (range - sizeof(*memr)) /
(BITS_PER_BYTE * __PAGE_SIZE + 1)

Also, the bitmap should be set to ones before calling map_free.

>       /* add to list */
>       memr->next = b->memr_head;
>       b->memr_head = memr;
> 

The patch should also include:
- changes to allocated_in_map function:
  1. rename page_num to page/page_va
  2. return unsigned long instead of int
  3. change the return value by using the correct bitmap array index and
offset in map word
- in map_alloc, correct values for curr_idx, start_off, end_idx, end_off
- in map_free, correct values for curr_idx, start_off, end_idx, end_off

Please include all the changes into a single patch and test them before
submission.


Thanks,
Costin

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel