From: Klaus Jensen <its@irrelevant.dk>
To: Niklas Cassel <Niklas.Cassel@wdc.com>
Cc: "kbusch@kernel.org" <kbusch@kernel.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Subject: Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed
Date: Mon, 7 Jun 2021 12:04:42 +0200 [thread overview]
Message-ID: <YL3vOlOoOoHodaxz@apples.localdomain> (raw)
In-Reply-To: <YL3t3RJ8exPXcYJc@x1-carbon>
[-- Attachment #1: Type: text/plain, Size: 12298 bytes --]
On Jun 7 09:58, Niklas Cassel wrote:
>On Mon, Jun 07, 2021 at 11:54:02AM +0200, Klaus Jensen wrote:
>> On Jun 1 07:30, Niklas Cassel wrote:
>> > On Mon, May 31, 2021 at 09:39:20PM +0200, Klaus Jensen wrote:
>> > > On May 31 15:42, Niklas Cassel wrote:
>> > > > On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
>> > > > > On May 28 11:05, Niklas Cassel wrote:
>> > > > > > From: Niklas Cassel <niklas.cassel@wdc.com>
>> > > > > >
>> > > > > > In the Zoned Namespace Command Set Specification, chapter
>> > > > > > 2.5.1 Managing resources
>> > > > > >
>> > > > > > "The controller may transition zones in the ZSIO:Implicitly Opened state
>> > > > > > to the ZSC:Closed state for resource management purposes."
>> > > > > >
>> > > > > > The word may in this sentence means that automatically transitioning
>> > > > > > an implicitly opened zone to closed is completely optional.
>> > > > > >
>> > > > > > Add a new parameter so that the user can control if this automatic
>> > > > > > transitioning should be performed or not.
>> > > > > >
>> > > > > > Being able to control this can help with verifying that e.g. a user-space
>> > > > > > program behaves properly even without this optional ZNS feature.
>> > > > > >
>> > > > > > The default value is set to true, in order to not change the existing
>> > > > > > behavior.
>> > > > > >
>> > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> > > > > > ---
>> > > > > > hw/nvme/ctrl.c | 9 ++++++++-
>> > > > > > hw/nvme/ns.c | 2 ++
>> > > > > > hw/nvme/nvme.h | 1 +
>> > > > > > 3 files changed, 11 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > > > > > index 40a7efcea9..d00f0297a5 100644
>> > > > > > --- a/hw/nvme/ctrl.c
>> > > > > > +++ b/hw/nvme/ctrl.c
>> > > > > > @@ -141,6 +141,11 @@
>> > > > > > *
>> > > > > > * zoned.cross_read=<enable RAZB, default: false>
>> > > > > > * Setting this property to true enables Read Across Zone Boundaries.
>> > > > > > + *
>> > > > > > + * zoned.auto_transition=<enable auto resource management, default: true>
>> > > > > > + * Indicates if zones in zone state implicitly opened can be
>> > > > > > + * automatically transitioned to zone state closed for resource
>> > > > > > + * management purposes.
>> > > > > > */
>> > > > > >
>> > > > > > #include "qemu/osdep.h"
>> > > > > > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > > > > /* fallthrough */
>> > > > > >
>> > > > > > case NVME_ZONE_STATE_CLOSED:
>> > > > > > - nvme_zrm_auto_transition_zone(ns);
>> > > > > > + if (ns->params.auto_transition_zones) {
>> > > > > > + nvme_zrm_auto_transition_zone(ns);
>> > > > > > + }
>> > > > > > status = nvme_aor_check(ns, act, 1);
>> > > > > > if (status) {
>> > > > > > return status;
>> > > > > > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>> > > > > > index 3fec9c6273..31dee43d30 100644
>> > > > > > --- a/hw/nvme/ns.c
>> > > > > > +++ b/hw/nvme/ns.c
>> > > > > > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
>> > > > > > params.max_open_zones, 0),
>> > > > > > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
>> > > > > > params.zd_extension_size, 0),
>> > > > > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
>> > > > > > + params.auto_transition_zones, true),
>> > > > > > DEFINE_PROP_END_OF_LIST(),
>> > > > > > };
>> > > > > >
>> > > > > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> > > > > > index 81a35cda14..bd86054db2 100644
>> > > > > > --- a/hw/nvme/nvme.h
>> > > > > > +++ b/hw/nvme/nvme.h
>> > > > > > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
>> > > > > > uint32_t max_active_zones;
>> > > > > > uint32_t max_open_zones;
>> > > > > > uint32_t zd_extension_size;
>> > > > > > + bool auto_transition_zones;
>> > > > > > } NvmeNamespaceParams;
>> > > > > >
>> > > > > > typedef struct NvmeNamespace {
>> > > > > > --
>> > > > > > 2.31.1
>> > > > > >
>> > > > >
>> > > > > Looks good Niklas!
>> > > > >
>> > > > > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> > > >
>> > > > In reality, it is the controller that does the auto transitioning.
>> > > >
>> > > > In theory, one namespace could be attached to two different controllers,
>> > > > and I guess, in that case, it depends on if the controller that we used
>> > > > when doing the write supports auto transitioning or not, that determines
>> > > > if a zone will be auto transitioned or not.
>> > > >
>> > > > If we were to change this to be a parameter of the controller instead
>> > > > of a parameter of the namespace, we would require to refactor a lot of
>> > > > code in the regular write path. As we currently don't have any NvmeRequest
>> > > > object in nvme_zrm_open_flags().
>> > > >
>> > > > Thoughts?
>> > > >
>> > >
>> > > I think you are right. This should be controller-specific behavior. I took
>> > > the liberty of moving the parameter; the refactor is minimal.
>> > >
>> > >
>> > > From: Niklas Cassel <niklas.cassel@wdc.com>
>> > >
>> > > In the Zoned Namespace Command Set Specification, chapter
>> > > 2.5.1 Managing resources
>> > >
>> > > "The controller may transition zones in the ZSIO:Implicitly Opened state
>> > > to the ZSC:Closed state for resource management purposes."
>> > >
>> > > The word may in this sentence means that automatically transitioning
>> > > an implicitly opened zone to closed is completely optional.
>> > >
>> > > Add a new parameter so that the user can control if this automatic
>> > > transitioning should be performed or not.
>> > >
>> > > Being able to control this can help with verifying that e.g. a user-space
>> > > program behaves properly even without this optional ZNS feature.
>> > >
>> > > The default value is set to true, in order to not change the existing
>> > > behavior.
>> > >
>> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> > > [k.jensen: moved parameter to controller]
>> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> > > ---
>> > > hw/nvme/nvme.h | 1 +
>> > > hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
>> > > 2 files changed, 23 insertions(+), 10 deletions(-)
>> > >
>> > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> > > index 81a35cda142b..93a7e0e5380e 100644
>> > > --- a/hw/nvme/nvme.h
>> > > +++ b/hw/nvme/nvme.h
>> > > @@ -382,6 +382,7 @@ typedef struct NvmeParams {
>> > > uint8_t vsl;
>> > > bool use_intel_id;
>> > > uint8_t zasl;
>> > > + bool auto_transition_zones;
>> > > bool legacy_cmb;
>> > > } NvmeParams;
>> > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > > index 40a7efcea914..8dd9cb2ccbf3 100644
>> > > --- a/hw/nvme/ctrl.c
>> > > +++ b/hw/nvme/ctrl.c
>> > > @@ -34,6 +34,7 @@
>> > > * aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
>> > > * mdts=<N[optional]>,vsl=<N[optional]>, \
>> > > * zoned.zasl=<N[optional]>, \
>> > > + * zoned.auto_transition=<on|off[optional]>, \
>> > > * subsys=<subsys_id>
>> > > * -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
>> > > * zoned=<true|false[optional]>, \
>> > > @@ -100,6 +101,11 @@
>> > > * the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
>> > > * defaulting to the value of `mdts`).
>> > > *
>> > > + * - `zoned.auto_transition`
>> > > + * Indicates if zones in zone state implicitly opened can be automatically
>> > > + * transitioned to zone state closed for resource management purposes.
>> > > + * Defaults to 'on'.
>> > > + *
>> > > * nvme namespace device parameters
>> > > * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > > * - `shared`
>> > > @@ -1686,8 +1692,8 @@ enum {
>> > > NVME_ZRM_AUTO = 1 << 0,
>> > > };
>> > > -static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > - int flags)
>> > > +static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
>> > > + NvmeZone *zone, int flags)
>> > > {
>> > > int act = 0;
>> > > uint16_t status;
>> > > @@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > /* fallthrough */
>> > > case NVME_ZONE_STATE_CLOSED:
>> > > - nvme_zrm_auto_transition_zone(ns);
>> > > + if (n->params.auto_transition_zones) {
>> > > + nvme_zrm_auto_transition_zone(ns);
>> > > + }
>> > > status = nvme_aor_check(ns, act, 1);
>> > > if (status) {
>> > > return status;
>> > > @@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > }
>> > > }
>> > > -static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
>> > > +static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
>> > > + NvmeZone *zone)
>> > > {
>> > > - return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
>> > > + return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
>> > > }
>> > > -static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
>> > > +static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
>> > > + NvmeZone *zone)
>> > > {
>> > > - return nvme_zrm_open_flags(ns, zone, 0);
>> > > + return nvme_zrm_open_flags(n, ns, zone, 0);
>> > > }
>> > > static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
>> > > @@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
>> > > goto invalid;
>> > > }
>> > > - status = nvme_zrm_auto(ns, zone);
>> > > + status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
>> > > if (status) {
>> > > goto invalid;
>> > > }
>> > > @@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
>> > > goto invalid;
>> > > }
>> > > - status = nvme_zrm_auto(ns, zone);
>> > > + status = nvme_zrm_auto(n, ns, zone);
>> > > if (status) {
>> > > goto invalid;
>> > > }
>> > > @@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
>> > > static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
>> > > NvmeZoneState state, NvmeRequest *req)
>> > > {
>> > > - return nvme_zrm_open(ns, zone);
>> > > + return nvme_zrm_open(nvme_ctrl(req), ns, zone);
>> > > }
>> > > static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
>> > > @@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
>> > > DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
>> > > DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
>> > > DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
>> > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
>> > > + params.auto_transition_zones, true),
>> > > DEFINE_PROP_END_OF_LIST(),
>> > > };
>> > > --
>> > > 2.31.1
>> > >
>> >
>> > Thanks a lot Klaus! I really appreciate it.
>> >
>> > My initial thought was to add a new flag in the enum where NVME_ZRM_AUTO is.
>> > But I think that would just make the code harder to read.
>> > You simply check the parameter directly, which is more obvious to the reader,
>> > so I think patch looks good!
>> >
>>
>> Can I add your reviewed-by on this? :)
>
>Yes, of course:
>
>Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
>
>However, in many projects you can't have a Reviewed-by that is the same as
>the author, but perhaps that is not the case in QEMU.
>
Ah doh - of course. I'll put my Sign-off and Reviewed-by on with you as
author of course ;) Forgot that I kept you as Author!
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2021-06-07 10:12 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-28 11:05 [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed Niklas Cassel
2021-05-28 11:22 ` Klaus Jensen
2021-05-31 15:42 ` Niklas Cassel
2021-05-31 19:39 ` Klaus Jensen
2021-06-01 7:30 ` Niklas Cassel
2021-06-07 9:54 ` Klaus Jensen
2021-06-07 9:58 ` Niklas Cassel
2021-06-07 10:04 ` Klaus Jensen [this message]
2021-06-07 10:08 ` Klaus Jensen
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=YL3vOlOoOoHodaxz@apples.localdomain \
--to=its@irrelevant.dk \
--cc=Niklas.Cassel@wdc.com \
--cc=kbusch@kernel.org \
--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.