| Message ID | 20190228061826.28305-7-stewart@linux.ibm.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Real Asynchronous flash access | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | fail | Signed-off-by missing |
On 02/28/2019 11:48 AM, Stewart Smith wrote: > The logic of a prepare call is to move the window (or at least start the > process of moving the window) so that when a read/write/erase call is > made, we don't have to wait for a IPMI round trip. > > For non-hiomap backends, one could use similar logic. > > --- > Not Signed-off-by yet as I'm totally not convinced Even I'm not convinced about this approach. Its still error prone. May be best is to split read/write/erase call to two parts (one initiates request to BMC and second one does post processing after getting data from BMC). That way we can remove synchronous message completely from these paths. Few comments based on current code. > --- > core/flash.c | 9 +++++++-- > libflash/blocklevel.c | 22 +++++++++++++++++++++ > libflash/blocklevel.h | 2 ++ > libflash/ipmi-hiomap.c | 44 ++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 73 insertions(+), 4 deletions(-) > > diff --git a/core/flash.c b/core/flash.c > index 04d99348515e..e267aaa0373d 100644 > --- a/core/flash.c > +++ b/core/flash.c > @@ -1,4 +1,4 @@ > -/* Copyright 2013-2018 IBM Corp. > +/* Copyright 2013-2019 IBM Corp. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -239,7 +239,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus > { > struct flash *flash = data; > uint64_t offset, buf, len; > - int rc; > + int rc = 0; > > offset = flash->async.pos; > buf = flash->async.buf; > @@ -254,6 +254,10 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus > len = MIN(flash->async.len, flash->block_size); > prlog(PR_TRACE, "Flash poll op %d len %llu\n", flash->async.op, len); > > + /* If we've done work to prepare, come back later */ > + if (blocklevel_raw_prepare(flash->bl, (flash->async.op!=FLASH_OP_READ), offset, len)) > + goto schedule; Need a way to differentiate errors vs waiting to get response from BMC. > + > switch (flash->async.op) { > case FLASH_OP_READ: > rc = blocklevel_raw_read(flash->bl, offset, (void *)buf, len); > @@ -274,6 +278,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus > flash->async.pos += len; > flash->async.buf += len; > flash->async.len -= len; > +schedule: > if (!rc && flash->async.len) { > /* > * We want to get called pretty much straight away, just have > diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c > index 5b57d3c6194f..9659c7fd68af 100644 .../... > > +static int ipmi_hiomap_prepare(struct blocklevel_device *bl, bool rw, > + uint64_t pos, uint64_t len) > +{ > + struct ipmi_hiomap *ctx; > + enum lpc_window_state want_state; > + uint64_t size; > + uint8_t command = HIOMAP_C_CREATE_READ_WINDOW; > + bool valid_state; > + bool is_read; > + int rc = 0; > + > + /* LPC is only 32bit */ > + if (pos > UINT_MAX || len > UINT_MAX) > + return FLASH_ERR_PARM_ERROR; > + > + if (rw) > + command = HIOMAP_C_CREATE_WRITE_WINDOW; > + > + ctx = container_of(bl, struct ipmi_hiomap, bl); > + > + is_read = (command == HIOMAP_C_CREATE_READ_WINDOW); > + want_state = is_read ? read_window : write_window; > + > + lock(&ctx->lock); > + valid_state = want_state == ctx->window_state; > + rc = hiomap_window_valid(ctx, pos, len); > + if (valid_state && !rc) { > + unlock(&ctx->lock); > + return 0; > + } > + unlock(&ctx->lock); > + > + rc = ipmi_hiomap_handle_events(ctx); > + if (rc) > + return rc; > + > + rc = hiomap_window_move(ctx, command, pos, len, &size); This still uses synchronous path. So not going to help much. -Vasant
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > On 02/28/2019 11:48 AM, Stewart Smith wrote: >> The logic of a prepare call is to move the window (or at least start the >> process of moving the window) so that when a read/write/erase call is >> made, we don't have to wait for a IPMI round trip. > >> For non-hiomap backends, one could use similar logic. >> >> --- >> Not Signed-off-by yet as I'm totally not convinced > > Even I'm not convinced about this approach. Its still error prone. > May be best is to split read/write/erase call to two parts (one initiates > request to BMC > and second one does post processing after getting data from BMC). > That way we can remove synchronous message completely from these > paths. Yeah, that was my thought as well... It involves more surgery to libflash though, which is annoying. > Few comments based on current code. > > >> --- >> core/flash.c | 9 +++++++-- >> libflash/blocklevel.c | 22 +++++++++++++++++++++ >> libflash/blocklevel.h | 2 ++ >> libflash/ipmi-hiomap.c | 44 ++++++++++++++++++++++++++++++++++++++++-- >> 4 files changed, 73 insertions(+), 4 deletions(-) >> >> diff --git a/core/flash.c b/core/flash.c >> index 04d99348515e..e267aaa0373d 100644 >> --- a/core/flash.c >> +++ b/core/flash.c >> @@ -1,4 +1,4 @@ >> -/* Copyright 2013-2018 IBM Corp. >> +/* Copyright 2013-2019 IBM Corp. >> * >> * Licensed under the Apache License, Version 2.0 (the "License"); >> * you may not use this file except in compliance with the License. >> @@ -239,7 +239,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus >> { >> struct flash *flash = data; >> uint64_t offset, buf, len; >> - int rc; >> + int rc = 0; >> >> offset = flash->async.pos; >> buf = flash->async.buf; >> @@ -254,6 +254,10 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus >> len = MIN(flash->async.len, flash->block_size); >> prlog(PR_TRACE, "Flash poll op %d len %llu\n", flash->async.op, len); >> >> + /* If we've done work to prepare, come back later */ >> + if (blocklevel_raw_prepare(flash->bl, (flash->async.op!=FLASH_OP_READ), offset, len)) >> + goto schedule; > > Need a way to differentiate errors vs waiting to get response from > BMC. yep. >> + >> switch (flash->async.op) { >> case FLASH_OP_READ: >> rc = blocklevel_raw_read(flash->bl, offset, (void *)buf, len); >> @@ -274,6 +278,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus >> flash->async.pos += len; >> flash->async.buf += len; >> flash->async.len -= len; >> +schedule: >> if (!rc && flash->async.len) { >> /* >> * We want to get called pretty much straight away, just have >> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c >> index 5b57d3c6194f..9659c7fd68af 100644 > > .../... > > >> >> +static int ipmi_hiomap_prepare(struct blocklevel_device *bl, bool rw, >> + uint64_t pos, uint64_t len) >> +{ >> + struct ipmi_hiomap *ctx; >> + enum lpc_window_state want_state; >> + uint64_t size; >> + uint8_t command = HIOMAP_C_CREATE_READ_WINDOW; >> + bool valid_state; >> + bool is_read; >> + int rc = 0; >> + >> + /* LPC is only 32bit */ >> + if (pos > UINT_MAX || len > UINT_MAX) >> + return FLASH_ERR_PARM_ERROR; >> + >> + if (rw) >> + command = HIOMAP_C_CREATE_WRITE_WINDOW; >> + >> + ctx = container_of(bl, struct ipmi_hiomap, bl); >> + >> + is_read = (command == HIOMAP_C_CREATE_READ_WINDOW); >> + want_state = is_read ? read_window : write_window; >> + >> + lock(&ctx->lock); >> + valid_state = want_state == ctx->window_state; >> + rc = hiomap_window_valid(ctx, pos, len); >> + if (valid_state && !rc) { >> + unlock(&ctx->lock); >> + return 0; >> + } >> + unlock(&ctx->lock); >> + >> + rc = ipmi_hiomap_handle_events(ctx); >> + if (rc) >> + return rc; >> + >> + rc = hiomap_window_move(ctx, command, pos, len, &size); > > This still uses synchronous path. So not going to help much. Yeah, it doesn't make a huge impact, but amazingly enough, there is *some* improvement (tens of ms)... but it needs work.
On 03/01/2019 06:06 AM, Stewart Smith wrote: > Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > >> On 02/28/2019 11:48 AM, Stewart Smith wrote: >>> The logic of a prepare call is to move the window (or at least start the >>> process of moving the window) so that when a read/write/erase call is >>> made, we don't have to wait for a IPMI round trip. > >>> For non-hiomap backends, one could use similar logic. >>> >>> --- >>> Not Signed-off-by yet as I'm totally not convinced >> >> Even I'm not convinced about this approach. Its still error prone. >> May be best is to split read/write/erase call to two parts (one initiates >> request to BMC >> and second one does post processing after getting data from BMC). >> That way we can remove synchronous message completely from these >> paths. > > Yeah, that was my thought as well... It involves more surgery to > libflash though, which is annoying. Unfortunately yes. May be its better to split this patchset into two. - Prepare core/flash.c code to asynchronous events (may be first four patch?) - Then change libflash code. .../.. >> >> This still uses synchronous path. So not going to help much. > > Yeah, it doesn't make a huge impact, but amazingly enough, there is > *some* improvement (tens of ms)... but it needs work. Also if we use asynchronous method, then it will completely avoid "Hardlock up issues". -Vasant
diff --git a/core/flash.c b/core/flash.c index 04d99348515e..e267aaa0373d 100644 --- a/core/flash.c +++ b/core/flash.c @@ -1,4 +1,4 @@ -/* Copyright 2013-2018 IBM Corp. +/* Copyright 2013-2019 IBM Corp. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -239,7 +239,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus { struct flash *flash = data; uint64_t offset, buf, len; - int rc; + int rc = 0; offset = flash->async.pos; buf = flash->async.buf; @@ -254,6 +254,10 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus len = MIN(flash->async.len, flash->block_size); prlog(PR_TRACE, "Flash poll op %d len %llu\n", flash->async.op, len); + /* If we've done work to prepare, come back later */ + if (blocklevel_raw_prepare(flash->bl, (flash->async.op!=FLASH_OP_READ), offset, len)) + goto schedule; + switch (flash->async.op) { case FLASH_OP_READ: rc = blocklevel_raw_read(flash->bl, offset, (void *)buf, len); @@ -274,6 +278,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus flash->async.pos += len; flash->async.buf += len; flash->async.len -= len; +schedule: if (!rc && flash->async.len) { /* * We want to get called pretty much straight away, just have diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c index 5b57d3c6194f..9659c7fd68af 100644 --- a/libflash/blocklevel.c +++ b/libflash/blocklevel.c @@ -92,6 +92,28 @@ static int release(struct blocklevel_device *bl) return rc; } +int blocklevel_raw_prepare(struct blocklevel_device *bl, bool rw, uint64_t pos, uint64_t len) +{ + int rc = 0; + + FL_DBG("%s: 0x%" PRIx64 "\t0x%" PRIx64 "\n", __func__, pos, len); + if (!bl) { + errno = EINVAL; + return FLASH_ERR_PARM_ERROR; + } + + rc = reacquire(bl); + if (rc) + return rc; + + if (bl->prepare) + rc = bl->prepare(bl, rw, pos, len); + + release(bl); + + return rc; +} + int blocklevel_raw_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint64_t len) { int rc; diff --git a/libflash/blocklevel.h b/libflash/blocklevel.h index ba42c83d003d..d4ce68cb5752 100644 --- a/libflash/blocklevel.h +++ b/libflash/blocklevel.h @@ -42,6 +42,7 @@ struct blocklevel_device { void *priv; int (*reacquire)(struct blocklevel_device *bl); int (*release)(struct blocklevel_device *bl); + int (*prepare)(struct blocklevel_device *bl, bool rw, uint64_t pos, uint64_t len); int (*read)(struct blocklevel_device *bl, uint64_t pos, void *buf, uint64_t len); int (*write)(struct blocklevel_device *bl, uint64_t pos, const void *buf, uint64_t len); int (*erase)(struct blocklevel_device *bl, uint64_t pos, uint64_t len); @@ -57,6 +58,7 @@ struct blocklevel_device { struct blocklevel_range ecc_prot; }; +int blocklevel_raw_prepare(struct blocklevel_device *bl, bool rw, uint64_t pos, uint64_t len); int blocklevel_raw_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint64_t len); int blocklevel_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint64_t len); int blocklevel_raw_write(struct blocklevel_device *bl, uint64_t pos, const void *buf, uint64_t len); diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c index 56492fa87067..8bd9d1762ce9 100644 --- a/libflash/ipmi-hiomap.c +++ b/libflash/ipmi-hiomap.c @@ -94,8 +94,6 @@ static int hiomap_window_valid(struct ipmi_hiomap *ctx, uint64_t pos, return FLASH_ERR_AGAIN; if (ctx->bmc_state & HIOMAP_E_WINDOW_RESET) return FLASH_ERR_AGAIN; - if (ctx->window_state == closed_window) - return FLASH_ERR_PARM_ERROR; if (pos < ctx->current.cur_pos) return FLASH_ERR_PARM_ERROR; if ((pos + len) > (ctx->current.cur_pos + ctx->current.size)) @@ -707,6 +705,47 @@ restore: return rc; } +static int ipmi_hiomap_prepare(struct blocklevel_device *bl, bool rw, + uint64_t pos, uint64_t len) +{ + struct ipmi_hiomap *ctx; + enum lpc_window_state want_state; + uint64_t size; + uint8_t command = HIOMAP_C_CREATE_READ_WINDOW; + bool valid_state; + bool is_read; + int rc = 0; + + /* LPC is only 32bit */ + if (pos > UINT_MAX || len > UINT_MAX) + return FLASH_ERR_PARM_ERROR; + + if (rw) + command = HIOMAP_C_CREATE_WRITE_WINDOW; + + ctx = container_of(bl, struct ipmi_hiomap, bl); + + is_read = (command == HIOMAP_C_CREATE_READ_WINDOW); + want_state = is_read ? read_window : write_window; + + lock(&ctx->lock); + valid_state = want_state == ctx->window_state; + rc = hiomap_window_valid(ctx, pos, len); + if (valid_state && !rc) { + unlock(&ctx->lock); + return 0; + } + unlock(&ctx->lock); + + rc = ipmi_hiomap_handle_events(ctx); + if (rc) + return rc; + + rc = hiomap_window_move(ctx, command, pos, len, &size); + + return (rc==0)? 1 : -(rc); +} + static int ipmi_hiomap_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint64_t len) { @@ -900,6 +939,7 @@ int ipmi_hiomap_init(struct blocklevel_device **bl) init_lock(&ctx->lock); + ctx->bl.prepare = &ipmi_hiomap_prepare; ctx->bl.read = &ipmi_hiomap_read; ctx->bl.write = &ipmi_hiomap_write; ctx->bl.erase = &ipmi_hiomap_erase;