| Message ID | 20230502130732.147210-2-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: uv-host: Fixups and extensions part 1 | expand |
Quoting Janosch Frank (2023-05-02 15:07:24) > The init memory has to be above 2G and 1M aligned but we're currently > aligning on 2G which means the allocations need a lot of unused > memory. I know I already gave my R-b here, but... > diff --git a/s390x/uv-host.c b/s390x/uv-host.c > index 33e6eec6..9dfaebd7 100644 > --- a/s390x/uv-host.c > +++ b/s390x/uv-host.c > @@ -500,14 +500,17 @@ static void test_config_create(void) > static void test_init(void) > { > int rc; > - uint64_t mem; > + uint64_t tmp; > > - /* Donated storage needs to be over 2GB */ > - mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL); ...maybe out of coffee, but can you point me to the place where we're aligning to 2G here? I only see alignment to 1M and your change only seems to rename mem to tmp: > + /* > + * Donated storage needs to be over 2GB, AREA_NORMAL does that > + * on s390x. > + */ > + tmp = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
On 5/10/23 10:51, Nico Boehr wrote: > Quoting Janosch Frank (2023-05-02 15:07:24) >> The init memory has to be above 2G and 1M aligned but we're currently >> aligning on 2G which means the allocations need a lot of unused >> memory. > > I know I already gave my R-b here, but... > >> diff --git a/s390x/uv-host.c b/s390x/uv-host.c >> index 33e6eec6..9dfaebd7 100644 >> --- a/s390x/uv-host.c >> +++ b/s390x/uv-host.c >> @@ -500,14 +500,17 @@ static void test_config_create(void) >> static void test_init(void) >> { >> int rc; >> - uint64_t mem; >> + uint64_t tmp; >> >> - /* Donated storage needs to be over 2GB */ >> - mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL); > > ...maybe out of coffee, but can you point me to the place where we're aligning > to 2G here? I only see alignment to 1M and your change only seems to rename > mem to tmp: > >> + /* >> + * Donated storage needs to be over 2GB, AREA_NORMAL does that >> + * on s390x. >> + */ This comment explains it :) Its a re-name of mem to tmp and an extension of this comment so it makes more sense. >> + tmp = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
Quoting Janosch Frank (2023-05-10 11:03:54) > On 5/10/23 10:51, Nico Boehr wrote: > > Quoting Janosch Frank (2023-05-02 15:07:24) > >> The init memory has to be above 2G and 1M aligned but we're currently > >> aligning on 2G which means the allocations need a lot of unused > >> memory. > > > > I know I already gave my R-b here, but... > > > >> diff --git a/s390x/uv-host.c b/s390x/uv-host.c > >> index 33e6eec6..9dfaebd7 100644 > >> --- a/s390x/uv-host.c > >> +++ b/s390x/uv-host.c > >> @@ -500,14 +500,17 @@ static void test_config_create(void) > >> static void test_init(void) > >> { > >> int rc; > >> - uint64_t mem; > >> + uint64_t tmp; > >> > >> - /* Donated storage needs to be over 2GB */ > >> - mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL); > > > > ...maybe out of coffee, but can you point me to the place where we're aligning > > to 2G here? I only see alignment to 1M and your change only seems to rename > > mem to tmp: > > > >> + /* > >> + * Donated storage needs to be over 2GB, AREA_NORMAL does that > >> + * on s390x. > >> + */ > > This comment explains it :) > Its a re-name of mem to tmp and an extension of this comment so it makes > more sense. Alright, thanks. I guess coffee level too low. Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
diff --git a/s390x/uv-host.c b/s390x/uv-host.c index 33e6eec6..9dfaebd7 100644 --- a/s390x/uv-host.c +++ b/s390x/uv-host.c @@ -500,14 +500,17 @@ static void test_config_create(void) static void test_init(void) { int rc; - uint64_t mem; + uint64_t tmp; - /* Donated storage needs to be over 2GB */ - mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL); + /* + * Donated storage needs to be over 2GB, AREA_NORMAL does that + * on s390x. + */ + tmp = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL); uvcb_init.header.len = sizeof(uvcb_init); uvcb_init.header.cmd = UVC_CMD_INIT_UV; - uvcb_init.stor_origin = mem; + uvcb_init.stor_origin = tmp; uvcb_init.stor_len = uvcb_qui.uv_base_stor_len; report_prefix_push("init"); @@ -528,14 +531,14 @@ static void test_init(void) rc = uv_call(0, (uint64_t)&uvcb_init); report(rc == 1 && (uvcb_init.header.rc == 0x104 || uvcb_init.header.rc == 0x105), "storage origin invalid"); - uvcb_init.stor_origin = mem; + uvcb_init.stor_origin = tmp; if (uvcb_init.stor_len >= HPAGE_SIZE) { uvcb_init.stor_origin = get_max_ram_size() - HPAGE_SIZE; rc = uv_call(0, (uint64_t)&uvcb_init); report(rc == 1 && uvcb_init.header.rc == 0x105, "storage + length invalid"); - uvcb_init.stor_origin = mem; + uvcb_init.stor_origin = tmp; } else { report_skip("storage + length invalid, stor_len < HPAGE_SIZE"); } @@ -544,7 +547,7 @@ static void test_init(void) rc = uv_call(0, (uint64_t)&uvcb_init); report(rc == 1 && uvcb_init.header.rc == 0x108, "storage below 2GB"); - uvcb_init.stor_origin = mem; + uvcb_init.stor_origin = tmp; smp_cpu_setup(1, PSW_WITH_CUR_MASK(cpu_loop)); rc = uv_call(0, (uint64_t)&uvcb_init); @@ -555,10 +558,12 @@ static void test_init(void) rc = uv_call(0, (uint64_t)&uvcb_init); report(rc == 0 && uvcb_init.header.rc == UVC_RC_EXECUTED, "successful"); - mem = (uint64_t)memalign(1UL << 31, uvcb_qui.uv_base_stor_len); + tmp = uvcb_init.stor_origin; + uvcb_init.stor_origin = (uint64_t)memalign_pages_flags(HPAGE_SIZE, uvcb_qui.uv_base_stor_len, AREA_NORMAL); rc = uv_call(0, (uint64_t)&uvcb_init); report(rc == 1 && uvcb_init.header.rc == 0x101, "double init"); - free((void *)mem); + free((void *)uvcb_init.stor_origin); + uvcb_init.stor_origin = tmp; report_prefix_pop(); }
The init memory has to be above 2G and 1M aligned but we're currently aligning on 2G which means the allocations need a lot of unused memory. Also the second block of memory was never actually used for the double init test since its address is never put into the uvcb. Let's fix that. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- s390x/uv-host.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)