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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).