From: Alistair Francis <alistair23@gmail.com>
To: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
"Niklas Cassel" <niklas.cassel@wdc.com>,
"Damien Le Moal" <damien.lemoal@wdc.com>,
Qemu-block <qemu-block@nongnu.org>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
"Keith Busch" <kbusch@kernel.org>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Maxim Levitsky" <mlevitsky@redhat.com>,
"Matias Bjorling" <matias.bjorling@wdc.com>
Subject: Re: [PATCH v2 11/18] hw/block/nvme: Introduce max active and open zone limits
Date: Tue, 30 Jun 2020 17:26:34 -0700 [thread overview]
Message-ID: <CAKmqyKOO1=VEa1bXGcsvEw2=x7Co2xy41CpjovdoYhazQckDtg@mail.gmail.com> (raw)
In-Reply-To: <20200617213415.22417-12-dmitry.fomichev@wdc.com>
On Wed, Jun 17, 2020 at 3:07 PM Dmitry Fomichev <dmitry.fomichev@wdc.com> wrote:
>
> Added two module properties, "max_active" and "max_open" to control
> the maximum number of zones that can be active or open. Once these
> variables are set to non-default values, the driver checks these
> limits during I/O and returns Too Many Active or Too Many Open
> command status if they are exceeded.
>
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++-
> hw/block/nvme.h | 4 ++
> 2 files changed, 185 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2e03b0b6ed..05a7cbcfcc 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -120,6 +120,87 @@ static void nvme_remove_zone(NvmeCtrl *n, NvmeNamespace *ns, NvmeZoneList *zl,
> zone->prev = zone->next = 0;
> }
>
> +/*
> + * Take the first zone out from a list, return NULL if the list is empty.
> + */
> +static NvmeZone *nvme_remove_zone_head(NvmeCtrl *n, NvmeNamespace *ns,
> + NvmeZoneList *zl)
> +{
> + NvmeZone *zone = nvme_peek_zone_head(ns, zl);
> +
> + if (zone) {
> + --zl->size;
> + if (zl->size == 0) {
> + zl->head = NVME_ZONE_LIST_NIL;
> + zl->tail = NVME_ZONE_LIST_NIL;
> + } else {
> + zl->head = zone->next;
> + ns->zone_array[zl->head].prev = NVME_ZONE_LIST_NIL;
> + }
> + zone->prev = zone->next = 0;
> + }
> +
> + return zone;
> +}
> +
> +/*
> + * Check if we can open a zone without exceeding open/active limits.
> + * AOR stands for "Active and Open Resources" (see TP 4053 section 2.5).
> + */
> +static int nvme_aor_check(NvmeCtrl *n, NvmeNamespace *ns,
> + uint32_t act, uint32_t opn)
> +{
> + if (n->params.max_active_zones != 0 &&
> + ns->nr_active_zones + act > n->params.max_active_zones) {
> + trace_pci_nvme_err_insuff_active_res(n->params.max_active_zones);
> + return NVME_ZONE_TOO_MANY_ACTIVE | NVME_DNR;
> + }
> + if (n->params.max_open_zones != 0 &&
> + ns->nr_open_zones + opn > n->params.max_open_zones) {
> + trace_pci_nvme_err_insuff_open_res(n->params.max_open_zones);
> + return NVME_ZONE_TOO_MANY_OPEN | NVME_DNR;
> + }
> +
> + return NVME_SUCCESS;
> +}
> +
> +static inline void nvme_aor_inc_open(NvmeCtrl *n, NvmeNamespace *ns)
> +{
> + assert(ns->nr_open_zones >= 0);
> + if (n->params.max_open_zones) {
> + ns->nr_open_zones++;
> + assert(ns->nr_open_zones <= n->params.max_open_zones);
> + }
> +}
> +
> +static inline void nvme_aor_dec_open(NvmeCtrl *n, NvmeNamespace *ns)
> +{
> + if (n->params.max_open_zones) {
> + assert(ns->nr_open_zones > 0);
> + ns->nr_open_zones--;
> + }
> + assert(ns->nr_open_zones >= 0);
> +}
> +
> +static inline void nvme_aor_inc_active(NvmeCtrl *n, NvmeNamespace *ns)
> +{
> + assert(ns->nr_active_zones >= 0);
> + if (n->params.max_active_zones) {
> + ns->nr_active_zones++;
> + assert(ns->nr_active_zones <= n->params.max_active_zones);
> + }
> +}
> +
> +static inline void nvme_aor_dec_active(NvmeCtrl *n, NvmeNamespace *ns)
> +{
> + if (n->params.max_active_zones) {
> + assert(ns->nr_active_zones > 0);
> + ns->nr_active_zones--;
> + assert(ns->nr_active_zones >= ns->nr_open_zones);
> + }
> + assert(ns->nr_active_zones >= 0);
> +}
> +
> static void nvme_assign_zone_state(NvmeCtrl *n, NvmeNamespace *ns,
> NvmeZone *zone, uint8_t state)
> {
> @@ -454,6 +535,24 @@ static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> }
>
> +static void nvme_auto_transition_zone(NvmeCtrl *n, NvmeNamespace *ns,
> + bool implicit, bool adding_active)
> +{
> + NvmeZone *zone;
> +
> + if (implicit && n->params.max_open_zones &&
> + ns->nr_open_zones == n->params.max_open_zones) {
> + zone = nvme_remove_zone_head(n, ns, ns->imp_open_zones);
> + if (zone) {
> + /*
> + * Automatically close this implicitly open zone.
> + */
> + nvme_aor_dec_open(n, ns);
> + nvme_assign_zone_state(n, ns, zone, NVME_ZONE_STATE_CLOSED);
> + }
> + }
> +}
> +
> static uint16_t nvme_check_zone_write(NvmeZone *zone, uint64_t slba,
> uint32_t nlb)
> {
> @@ -531,6 +630,23 @@ static uint16_t nvme_check_zone_read(NvmeCtrl *n, NvmeZone *zone, uint64_t slba,
> return status;
> }
>
> +static uint16_t nvme_auto_open_zone(NvmeCtrl *n, NvmeNamespace *ns,
> + NvmeZone *zone)
> +{
> + uint16_t status = NVME_SUCCESS;
> + uint8_t zs = nvme_get_zone_state(zone);
> +
> + if (zs == NVME_ZONE_STATE_EMPTY) {
> + nvme_auto_transition_zone(n, ns, true, true);
> + status = nvme_aor_check(n, ns, 1, 1);
> + } else if (zs == NVME_ZONE_STATE_CLOSED) {
> + nvme_auto_transition_zone(n, ns, true, false);
> + status = nvme_aor_check(n, ns, 0, 1);
> + }
> +
> + return status;
> +}
> +
> static uint64_t nvme_finalize_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
> NvmeZone *zone, uint32_t nlb)
> {
> @@ -543,7 +659,11 @@ static uint64_t nvme_finalize_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
> switch (zs) {
> case NVME_ZONE_STATE_IMPLICITLY_OPEN:
> case NVME_ZONE_STATE_EXPLICITLY_OPEN:
> + nvme_aor_dec_open(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_CLOSED:
> + nvme_aor_dec_active(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_EMPTY:
> break;
> default:
> @@ -553,7 +673,10 @@ static uint64_t nvme_finalize_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
> } else {
> switch (zs) {
> case NVME_ZONE_STATE_EMPTY:
> + nvme_aor_inc_active(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_CLOSED:
> + nvme_aor_inc_open(n, ns);
> nvme_assign_zone_state(n, ns, zone,
> NVME_ZONE_STATE_IMPLICITLY_OPEN);
> }
> @@ -636,6 +759,11 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
> zone->d.wp);
> return NVME_ZONE_INVALID_WRITE | NVME_DNR;
> }
> +
> + status = nvme_auto_open_zone(n, ns, zone);
> + if (status != NVME_SUCCESS) {
> + return status;
> + }
> }
>
> block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
> @@ -709,6 +837,11 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
> zone->d.wp);
> return NVME_ZONE_INVALID_WRITE | NVME_DNR;
> }
> +
> + status = nvme_auto_open_zone(n, ns, zone);
> + if (status != NVME_SUCCESS) {
> + return status;
> + }
> } else {
> status = nvme_check_zone_read(n, zone, slba, nlb,
> n->params.cross_zone_read);
> @@ -804,9 +937,27 @@ static uint16_t nvme_get_mgmt_zone_slba_idx(NvmeCtrl *n, NvmeNamespace *ns,
> static uint16_t nvme_open_zone(NvmeCtrl *n, NvmeNamespace *ns,
> NvmeZone *zone, uint8_t state)
> {
> + uint16_t status;
> +
> switch (state) {
> case NVME_ZONE_STATE_EMPTY:
> + nvme_auto_transition_zone(n, ns, false, true);
> + status = nvme_aor_check(n, ns, 1, 0);
> + if (status != NVME_SUCCESS) {
> + return status;
> + }
> + nvme_aor_inc_active(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_CLOSED:
> + status = nvme_aor_check(n, ns, 0, 1);
> + if (status != NVME_SUCCESS) {
> + if (state == NVME_ZONE_STATE_EMPTY) {
> + nvme_aor_dec_active(n, ns);
> + }
> + return status;
> + }
> + nvme_aor_inc_open(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_IMPLICITLY_OPEN:
> nvme_assign_zone_state(n, ns, zone, NVME_ZONE_STATE_EXPLICITLY_OPEN);
> /* fall through */
> @@ -828,6 +979,7 @@ static uint16_t nvme_close_zone(NvmeCtrl *n, NvmeNamespace *ns,
> switch (state) {
> case NVME_ZONE_STATE_EXPLICITLY_OPEN:
> case NVME_ZONE_STATE_IMPLICITLY_OPEN:
> + nvme_aor_dec_open(n, ns);
> nvme_assign_zone_state(n, ns, zone, NVME_ZONE_STATE_CLOSED);
> /* fall through */
> case NVME_ZONE_STATE_CLOSED:
> @@ -849,7 +1001,11 @@ static uint16_t nvme_finish_zone(NvmeCtrl *n, NvmeNamespace *ns,
> switch (state) {
> case NVME_ZONE_STATE_EXPLICITLY_OPEN:
> case NVME_ZONE_STATE_IMPLICITLY_OPEN:
> + nvme_aor_dec_open(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_CLOSED:
> + nvme_aor_dec_active(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_EMPTY:
> zone->d.wp = nvme_zone_wr_boundary(zone);
> nvme_assign_zone_state(n, ns, zone, NVME_ZONE_STATE_FULL);
> @@ -874,7 +1030,11 @@ static uint16_t nvme_reset_zone(NvmeCtrl *n, NvmeNamespace *ns,
> switch (state) {
> case NVME_ZONE_STATE_EXPLICITLY_OPEN:
> case NVME_ZONE_STATE_IMPLICITLY_OPEN:
> + nvme_aor_dec_open(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_CLOSED:
> + nvme_aor_dec_active(n, ns);
> + /* fall through */
> case NVME_ZONE_STATE_FULL:
> zone->d.wp = zone->d.zslba;
> nvme_assign_zone_state(n, ns, zone, NVME_ZONE_STATE_EMPTY);
> @@ -2412,6 +2572,15 @@ static void nvme_zoned_init_ctrl(NvmeCtrl *n, Error **errp)
> uint64_t zone_size = 0, capacity;
> uint32_t nz;
>
> + if (n->params.max_open_zones < 0) {
> + error_setg(errp, "invalid max_open_zones value");
> + return;
> + }
> + if (n->params.max_active_zones < 0) {
> + error_setg(errp, "invalid max_active_zones value");
> + return;
> + }
> +
> if (n->params.zone_size) {
> zone_size = n->params.zone_size;
> } else {
> @@ -2435,6 +2604,14 @@ static void nvme_zoned_init_ctrl(NvmeCtrl *n, Error **errp)
> n->num_zones = nz;
> n->zone_array_size = sizeof(NvmeZone) * nz;
>
> + /* Make sure that the values of all Zoned Command Set properties are sane */
> + if (n->params.max_open_zones > nz) {
> + n->params.max_open_zones = nz;
> + }
> + if (n->params.max_active_zones > nz) {
> + n->params.max_active_zones = nz;
> + }
Should there be some warning here? You are overwriting the property
that was set by the board, it seems like you should tell someone.
Alistair
> +
> return;
> }
>
> @@ -2452,8 +2629,8 @@ static int nvme_zoned_init_ns(NvmeCtrl *n, NvmeNamespace *ns, int lba_index,
> ns->id_ns_zoned = g_malloc0(sizeof(*ns->id_ns_zoned));
>
> /* MAR/MOR are zeroes-based, 0xffffffff means no limit */
> - ns->id_ns_zoned->mar = 0xffffffff;
> - ns->id_ns_zoned->mor = 0xffffffff;
> + ns->id_ns_zoned->mar = cpu_to_le32(n->params.max_active_zones - 1);
> + ns->id_ns_zoned->mor = cpu_to_le32(n->params.max_open_zones - 1);
> ns->id_ns_zoned->zoc = 0;
> ns->id_ns_zoned->ozcs = n->params.cross_zone_read ? 0x01 : 0x00;
>
> @@ -2813,6 +2990,8 @@ static Property nvme_props[] = {
> DEFINE_PROP_UINT64("zone_size", NvmeCtrl, params.zone_size, 512),
> DEFINE_PROP_UINT64("zone_capacity", NvmeCtrl, params.zone_capacity, 512),
> DEFINE_PROP_UINT32("zone_append_max_size", NvmeCtrl, params.zamds_bs, 0),
> + DEFINE_PROP_INT32("max_active", NvmeCtrl, params.max_active_zones, 0),
> + DEFINE_PROP_INT32("max_open", NvmeCtrl, params.max_open_zones, 0),
> DEFINE_PROP_BOOL("cross_zone_read", NvmeCtrl, params.cross_zone_read, true),
> DEFINE_PROP_UINT8("fill_pattern", NvmeCtrl, params.fill_pattern, 0),
> DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 2c932b5e29..f5a4679702 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -19,6 +19,8 @@ typedef struct NvmeParams {
> uint32_t zamds_bs;
> uint64_t zone_size;
> uint64_t zone_capacity;
> + int32_t max_active_zones;
> + int32_t max_open_zones;
> } NvmeParams;
>
> typedef struct NvmeAsyncEvent {
> @@ -103,6 +105,8 @@ typedef struct NvmeNamespace {
> NvmeZoneList *imp_open_zones;
> NvmeZoneList *closed_zones;
> NvmeZoneList *full_zones;
> + int32_t nr_open_zones;
> + int32_t nr_active_zones;
> } NvmeNamespace;
>
> static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
> --
> 2.21.0
>
>
next prev parent reply other threads:[~2020-07-01 0:37 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-17 21:33 [PATCH v2 00/18] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set Dmitry Fomichev
2020-06-17 21:33 ` [PATCH v2 01/18] hw/block/nvme: Move NvmeRequest has_sg field to a bit flag Dmitry Fomichev
2020-06-30 0:56 ` Alistair Francis
2020-06-30 4:09 ` Klaus Jensen
2020-06-17 21:33 ` [PATCH v2 02/18] hw/block/nvme: Define 64 bit cqe.result Dmitry Fomichev
2020-06-30 0:58 ` Alistair Francis
2020-06-30 4:15 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 03/18] hw/block/nvme: Clean up unused AER definitions Dmitry Fomichev
2020-06-30 1:00 ` Alistair Francis
2020-06-30 4:40 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 04/18] hw/block/nvme: Add Commands Supported and Effects log Dmitry Fomichev
2020-06-30 1:35 ` Alistair Francis
2020-06-30 4:46 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 05/18] hw/block/nvme: Introduce the Namespace Types definitions Dmitry Fomichev
2020-06-30 2:12 ` Alistair Francis
2020-06-30 10:02 ` Niklas Cassel
2020-06-30 17:02 ` Keith Busch
2020-06-30 4:57 ` Klaus Jensen
2020-06-30 16:04 ` Niklas Cassel
2020-06-17 21:34 ` [PATCH v2 06/18] hw/block/nvme: Define trace events related to NS Types Dmitry Fomichev
2020-06-30 10:20 ` Klaus Jensen
2020-06-30 20:18 ` Alistair Francis
2020-06-17 21:34 ` [PATCH v2 07/18] hw/block/nvme: Add support for Namespace Types Dmitry Fomichev
2020-06-30 11:31 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 08/18] hw/block/nvme: Make Zoned NS Command Set definitions Dmitry Fomichev
2020-06-30 11:44 ` Klaus Jensen
2020-06-30 12:08 ` Klaus Jensen
2020-06-30 22:11 ` Alistair Francis
2020-06-17 21:34 ` [PATCH v2 09/18] hw/block/nvme: Define Zoned NS Command Set trace events Dmitry Fomichev
2020-06-30 12:14 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 10/18] hw/block/nvme: Support Zoned Namespace Command Set Dmitry Fomichev
2020-06-30 13:31 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 11/18] hw/block/nvme: Introduce max active and open zone limits Dmitry Fomichev
2020-07-01 0:26 ` Alistair Francis [this message]
2020-07-01 6:41 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 12/18] hw/block/nvme: Simulate Zone Active excursions Dmitry Fomichev
2020-07-01 0:30 ` Alistair Francis
2020-07-01 6:12 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 13/18] hw/block/nvme: Set Finish/Reset Zone Recommended attributes Dmitry Fomichev
2020-07-01 16:23 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 14/18] hw/block/nvme: Generate zone AENs Dmitry Fomichev
2020-07-01 11:44 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 15/18] hw/block/nvme: Support Zone Descriptor Extensions Dmitry Fomichev
2020-07-01 16:32 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 16/18] hw/block/nvme: Add injection of Offline/Read-Only zones Dmitry Fomichev
2020-06-17 21:34 ` [PATCH v2 17/18] hw/block/nvme: Use zone metadata file for persistence Dmitry Fomichev
2020-07-01 17:26 ` Klaus Jensen
2020-06-17 21:34 ` [PATCH v2 18/18] hw/block/nvme: Document zoned parameters in usage text Dmitry Fomichev
2020-06-29 20:26 ` [PATCH v2 00/18] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set Dmitry Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKmqyKOO1=VEa1bXGcsvEw2=x7Co2xy41CpjovdoYhazQckDtg@mail.gmail.com' \
--to=alistair23@gmail.com \
--cc=damien.lemoal@wdc.com \
--cc=dmitry.fomichev@wdc.com \
--cc=kbusch@kernel.org \
--cc=kwolf@redhat.com \
--cc=matias.bjorling@wdc.com \
--cc=mlevitsky@redhat.com \
--cc=niklas.cassel@wdc.com \
--cc=philmd@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.