| Message ID | 20240312130331.78418-7-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt1 | expand |
Hi Luca, On 12/03/2024 14:03, Luca Fancellu wrote: > > > The function find_unallocated_memory is using the same code to > loop through 3 structure of the same type, in order to avoid > code duplication, rework the code to have only one loop that > goes through all the structures. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/domain_build.c | 62 ++++++++++--------------------------- > 1 file changed, 17 insertions(+), 45 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index b254f252e7cb..d0f2ac6060eb 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, > static int __init find_unallocated_memory(const struct kernel_info *kinfo, > struct membanks *ext_regions) > { > - const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo); > - const struct membanks *mem = bootinfo_get_mem(); > - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); > + const struct membanks *mem_banks[] = { > + bootinfo_get_mem(), > + kernel_info_get_mem(kinfo), > + bootinfo_get_reserved_mem(), > + }; > struct rangeset *unalloc_mem; > paddr_t start, end; > - unsigned int i; > + unsigned int i, j; > int res; > > dt_dprintk("Find unallocated memory for extended regions\n"); > @@ -883,50 +885,20 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > if ( !unalloc_mem ) > return -ENOMEM; > > - /* Start with all available RAM */ > - for ( i = 0; i < mem->nr_banks; i++ ) > - { > - start = mem->bank[i].start; > - end = mem->bank[i].start + mem->bank[i].size; > - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), > - PFN_DOWN(end - 1)); > - if ( res ) > - { > - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", > - start, end); > - goto out; > - } > - } > - > - /* Remove RAM assigned to Dom0 */ > - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) > - { > - start = kinfo_mem->bank[i].start; > - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; > - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), > - PFN_DOWN(end - 1)); > - if ( res ) > + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) > + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) It might be a matter of personal opinion, but I would actually prefer the current code that looks simpler/neater (the steps are clear) to me. I'd like to know other maintainers opinion. ~Michal
> On 20 Mar 2024, at 10:57, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > > On 12/03/2024 14:03, Luca Fancellu wrote: >> >> >> The function find_unallocated_memory is using the same code to >> loop through 3 structure of the same type, in order to avoid >> code duplication, rework the code to have only one loop that >> goes through all the structures. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> xen/arch/arm/domain_build.c | 62 ++++++++++--------------------------- >> 1 file changed, 17 insertions(+), 45 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index b254f252e7cb..d0f2ac6060eb 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, >> static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> struct membanks *ext_regions) >> { >> - const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo); >> - const struct membanks *mem = bootinfo_get_mem(); >> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); >> + const struct membanks *mem_banks[] = { >> + bootinfo_get_mem(), >> + kernel_info_get_mem(kinfo), >> + bootinfo_get_reserved_mem(), >> + }; >> struct rangeset *unalloc_mem; >> paddr_t start, end; >> - unsigned int i; >> + unsigned int i, j; >> int res; >> >> dt_dprintk("Find unallocated memory for extended regions\n"); >> @@ -883,50 +885,20 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> if ( !unalloc_mem ) >> return -ENOMEM; >> >> - /* Start with all available RAM */ >> - for ( i = 0; i < mem->nr_banks; i++ ) >> - { >> - start = mem->bank[i].start; >> - end = mem->bank[i].start + mem->bank[i].size; >> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), >> - PFN_DOWN(end - 1)); >> - if ( res ) >> - { >> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", >> - start, end); >> - goto out; >> - } >> - } >> - >> - /* Remove RAM assigned to Dom0 */ >> - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) >> - { >> - start = kinfo_mem->bank[i].start; >> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; >> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), >> - PFN_DOWN(end - 1)); >> - if ( res ) >> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) >> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) > It might be a matter of personal opinion, but I would actually prefer the current code > that looks simpler/neater (the steps are clear) to me. I'd like to know other maintainers opinion. Ok, I’ll wait the other maintainers then, I’m going to use this construct in other part of the code to simplify and remove duplication so it would be important to know if It’s desirable or not. Maybe your opinion could change with a proper comment on top of the structure and the loop, listing the operation performed in order? 1) Start with all available RAM 2) Remove RAM assigned to Dom0 3) Remove reserved mem <later> 4) Remove static shared memory regions Cheers, Luca > > ~Michal
Hi Luca, On 20/03/2024 15:53, Luca Fancellu wrote: > > >> On 20 Mar 2024, at 10:57, Michal Orzel <michal.orzel@amd.com> wrote: >> >> Hi Luca, >> >> On 12/03/2024 14:03, Luca Fancellu wrote: >>> >>> >>> The function find_unallocated_memory is using the same code to >>> loop through 3 structure of the same type, in order to avoid >>> code duplication, rework the code to have only one loop that >>> goes through all the structures. >>> >>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >>> --- >>> xen/arch/arm/domain_build.c | 62 ++++++++++--------------------------- >>> 1 file changed, 17 insertions(+), 45 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index b254f252e7cb..d0f2ac6060eb 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, >>> static int __init find_unallocated_memory(const struct kernel_info *kinfo, >>> struct membanks *ext_regions) >>> { >>> - const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo); >>> - const struct membanks *mem = bootinfo_get_mem(); >>> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); >>> + const struct membanks *mem_banks[] = { >>> + bootinfo_get_mem(), >>> + kernel_info_get_mem(kinfo), >>> + bootinfo_get_reserved_mem(), >>> + }; >>> struct rangeset *unalloc_mem; >>> paddr_t start, end; >>> - unsigned int i; >>> + unsigned int i, j; >>> int res; >>> >>> dt_dprintk("Find unallocated memory for extended regions\n"); >>> @@ -883,50 +885,20 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >>> if ( !unalloc_mem ) >>> return -ENOMEM; >>> >>> - /* Start with all available RAM */ >>> - for ( i = 0; i < mem->nr_banks; i++ ) >>> - { >>> - start = mem->bank[i].start; >>> - end = mem->bank[i].start + mem->bank[i].size; >>> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), >>> - PFN_DOWN(end - 1)); >>> - if ( res ) >>> - { >>> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", >>> - start, end); >>> - goto out; >>> - } >>> - } >>> - >>> - /* Remove RAM assigned to Dom0 */ >>> - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) >>> - { >>> - start = kinfo_mem->bank[i].start; >>> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; >>> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), >>> - PFN_DOWN(end - 1)); >>> - if ( res ) >>> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) >>> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) >> It might be a matter of personal opinion, but I would actually prefer the current code >> that looks simpler/neater (the steps are clear) to me. I'd like to know other maintainers opinion. > > Ok, I’ll wait the other maintainers then, I’m going to use this construct in other part > of the code to simplify and remove duplication so it would be important to know if > It’s desirable or not. > > Maybe your opinion could change with a proper comment on top of the structure and the loop, > listing the operation performed in order? > > 1) Start with all available RAM > 2) Remove RAM assigned to Dom0 > 3) Remove reserved mem > <later> > 4) Remove static shared memory regions Yes, that would help with the overall readability. ~Michal
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index b254f252e7cb..d0f2ac6060eb 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, static int __init find_unallocated_memory(const struct kernel_info *kinfo, struct membanks *ext_regions) { - const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo); - const struct membanks *mem = bootinfo_get_mem(); - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); + const struct membanks *mem_banks[] = { + bootinfo_get_mem(), + kernel_info_get_mem(kinfo), + bootinfo_get_reserved_mem(), + }; struct rangeset *unalloc_mem; paddr_t start, end; - unsigned int i; + unsigned int i, j; int res; dt_dprintk("Find unallocated memory for extended regions\n"); @@ -883,50 +885,20 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, if ( !unalloc_mem ) return -ENOMEM; - /* Start with all available RAM */ - for ( i = 0; i < mem->nr_banks; i++ ) - { - start = mem->bank[i].start; - end = mem->bank[i].start + mem->bank[i].size; - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), - PFN_DOWN(end - 1)); - if ( res ) - { - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); - goto out; - } - } - - /* Remove RAM assigned to Dom0 */ - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) - { - start = kinfo_mem->bank[i].start; - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), - PFN_DOWN(end - 1)); - if ( res ) + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) { - printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); - goto out; - } - } - - /* Remove reserved-memory regions */ - for ( i = 0; i < reserved_mem->nr_banks; i++ ) - { - start = reserved_mem->bank[i].start; - end = reserved_mem->bank[i].start + reserved_mem->bank[i].size; - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), + start = mem_banks[i]->bank[j].start; + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size; + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end - 1)); - if ( res ) - { - printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); - goto out; + if ( res ) + { + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", + start, end); + goto out; + } } - } /* Remove grant table region */ if ( kinfo->gnttab_size )
The function find_unallocated_memory is using the same code to loop through 3 structure of the same type, in order to avoid code duplication, rework the code to have only one loop that goes through all the structures. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/domain_build.c | 62 ++++++++++--------------------------- 1 file changed, 17 insertions(+), 45 deletions(-)