Re: [RFC PATCH 2/4] lmb: Tighten up the code in lmb_add_region_flags()

On Thu, 1 Aug 2024 at 21:42, Simon Glass sjg@chromium.org wrote:

Hi Sughosh,

On Thu, 1 Aug 2024 at 08:58, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

On Tue, 30 Jul 2024 at 20:10, Simon Glass sjg@chromium.org wrote:

...

This function has more special cases than it needs. Simplify it to reduce code size and complexity.

Signed-off-by: Simon Glass sjg@chromium.org

lib/lmb.c | 57 +++++++++++++++++++------------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c index c11ce308c5b..83b060a2f4d 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -396,14 +396,6 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, if (alist_err(lmb_rgn_lst)) return -1;

•   if (alist_empty(lmb_rgn_lst)) {
  

•           rgn[0].base = base;
  

•           rgn[0].size = size;
  

•           rgn[0].flags = flags;
  

•           lmb_rgn_lst->count = 1;
  

•           return 0;
  

•   }
  

•   /* First try and coalesce this LMB with another. */
    for (i = 0; i < lmb_rgn_lst->count; i++) {
            phys_addr_t rgnbase = rgn[i].base;
  

@@ -448,50 +440,39 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, } }

•   if (i < lmb_rgn_lst->count - 1 &&
  

•       rgn[i].flags == rgn[i + 1].flags) {
  

•           if (lmb_regions_adjacent(lmb_rgn_lst, i, i + 1)) {
  

•                   lmb_coalesce_regions(lmb_rgn_lst, i, i + 1);
  

•                   coalesced++;
  

•           } else if (lmb_regions_overlap(lmb_rgn_lst, i, i + 1)) {
  

•                   /* fix overlapping area */
  

•                   lmb_fix_over_lap_regions(lmb_rgn_lst, i, i + 1);
  

•                   coalesced++;
  

•   if (lmb_rgn_lst->count && i < lmb_rgn_lst->count - 1) {
  

•           rgn = lmb_rgn_lst->data;
  

•           if (rgn[i].flags == rgn[i + 1].flags) {
  

•                   if (lmb_regions_adjacent(lmb_rgn_lst, i, i + 1)) {
  

•                           lmb_coalesce_regions(lmb_rgn_lst, i, i + 1);
  

•                           coalesced++;
  

•                   } else if (lmb_regions_overlap(lmb_rgn_lst, i, i + 1)) {
  

•                           /* fix overlapping area */
  

•                           lmb_fix_over_lap_regions(lmb_rgn_lst, i, i + 1);
  

•                           coalesced++;
  

•                   }
            }
    }
  
    if (coalesced)
            return coalesced;
  
  

•   if (alist_full(lmb_rgn_lst)) {
  

•           if (!alist_expand_by(lmb_rgn_lst, lmb_rgn_lst->alloc * 2))
  

•                   return -1;
  

•           else
  

•                   rgn = lmb_rgn_lst->data;
  

•   }
  

•   if (!alist_add_placeholder(lmb_rgn_lst))
  

•           return -1;
  

•   rgn = lmb_rgn_lst->data;
  

I think the above should have a check for alist_full(), and then add to the list if full. Else we simply go on adding to the list on every new node that gets added.

At this point in the function, we know that we are adding a new entry. We could have an anlist_insert() perhaps?

...

...

    /* Couldn't coalesce the LMB, so add it to the sorted table. */
    for (i = lmb_rgn_lst->count - 1; i >= 0; i--) {

•           if (base < rgn[i].base) {
  

•                   rgn[i + 1].base = rgn[i].base;
  

•                   rgn[i + 1].size = rgn[i].size;
  

•                   rgn[i + 1].flags = rgn[i].flags;
  

•           if (i && base < rgn[i - 1].base) {
  

•                   rgn[i] = rgn[i - 1];
            } else {
  

•                   rgn[i + 1].base = base;
  

•                   rgn[i + 1].size = size;
  

•                   rgn[i + 1].flags = flags;
  

•                   rgn[i].base = base;
  

•                   rgn[i].size = size;
  

•                   rgn[i].flags = flags;
                    break;
            }
    }
  

With the logic that you put above, should the loop not have 'i' initialised to lmb_rgn_lst-count? I mean

for (i = lmb_rgn_lst->count; i >= 0; i--)

Else we are overwriting one region?

It starts by doing the assignment rgn[count - 1 + 1] = rgn[count - 1], which I believe is correct?

That was the earlier logic, but now this is being replaced by

+ if (i && base < rgn[i - 1].base) { + rgn[i] = rgn[i - 1];

Which means 'i' should be initialised to lmb_rgn_cnt->count? If you see the patch, the for loop initialisation is not being changed.

-sughosh

...

...

•   if (base < rgn[0].base) {
  

•           rgn[0].base = base;
  

•           rgn[0].size = size;
  

•           rgn[0].flags = flags;
  

•   }
  

•   lmb_rgn_lst->count++;
  

•   return 0;
  

}

-- 2.34.1

Regards, SImon