Re: [PATCH v2 3/3] nvme: Add physical writes/reads from OCP log
On Nov 16 17:19, Joel Granados wrote: > On Tue, Nov 15, 2022 at 12:26:17PM +0100, Klaus Jensen wrote: > > On Nov 14 14:50, Joel Granados wrote: > > > > > > +static uint16_t nvme_vendor_specific_log(uint8_t lid, NvmeCtrl *n, > > > uint8_t rae, > > > + uint32_t buf_len, uint64_t off, > > > + NvmeRequest *req) > > > > `NvmeCtrl *n` must be first parameter. > Any reason why this is the case? I'll change it in my code, but would be > nice to understand the reason. > No other reason than consistency with existing code. signature.asc Description: PGP signature
Re: [PATCH v2 3/3] nvme: Add physical writes/reads from OCP log
On Tue, Nov 15, 2022 at 12:26:17PM +0100, Klaus Jensen wrote:
> On Nov 14 14:50, Joel Granados wrote:
> > In order to evaluate write amplification factor (WAF) within the storage
> > stack it is important to know the number of bytes written to the
> > controller. The existing SMART log value of Data Units Written is too
> > coarse (given in units of 500 Kb) and so we add the SMART health
> > information extended from the OCP specification (given in units of bytes).
> >
> > To accomodate different vendor specific specifications like OCP, we add a
> > multiplexing function (nvme_vendor_specific_log) which will route to the
> > different log functions based on arguments and log ids. We only return the
> > OCP extended smart log when the command is 0xC0 and ocp has been turned on
> > in the args.
> >
> > Though we add the whole nvme smart log extended structure, we only populate
> > the physical_media_units_{read,written}, log_page_version and
> > log_page_uuid.
> >
> > Signed-off-by: Joel Granados
> >
> > squash with main
> >
> > Signed-off-by: Joel Granados
>
> Looks like you slightly messed up the squash ;)
oops. that is my bad
>
> Also, squash the previous patch (adding the ocp parameter) into this.
Here I wanted to keep the introduction of the argument separate. In any
case, I'll squash it with the other one.
> Please add a note in the documentation (docs/system/devices/nvme.rst)
> about this parameter.
Of course. I always forget documentation. I'll add it under the
"Controller Emulation" section and I'll call it ``ocp``
>
> > ---
> > hw/nvme/ctrl.c | 56
> > include/block/nvme.h | 36
> > 2 files changed, 92 insertions(+)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 220683201a..5e6a8150a2 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -4455,6 +4455,42 @@ static void nvme_set_blk_stats(NvmeNamespace *ns,
> > struct nvme_stats *stats)
> > stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
> > }
> >
> > +static uint16_t nvme_ocp_extended_smart_info(NvmeCtrl *n, uint8_t rae,
> > + uint32_t buf_len, uint64_t
> > off,
> > + NvmeRequest *req)
> > +{
> > +NvmeNamespace *ns = NULL;
> > +NvmeSmartLogExtended smart_ext = { 0 };
> > +struct nvme_stats stats = { 0 };
> > +uint32_t trans_len;
> > +
> > +if (off >= sizeof(smart_ext)) {
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> > +
> > +// Accumulate all stats from all namespaces
>
> Use /* lower-case and no period */ for one sentence, one line comments.
>
> I think scripts/checkpatch.pl picks this up.
There is a checkpatch like in the kernel. Fantastic! I'll make a note to
use it from now on.
>
> > +for (int i = 1; i <= NVME_MAX_NAMESPACES; i++) {
> > +ns = nvme_ns(n, i);
> > +if (ns)
> > +{
>
> Paranthesis go on the same line as the `if`.
of course
>
> > +nvme_set_blk_stats(ns, &stats);
> > +}
> > +}
> > +
> > +smart_ext.physical_media_units_written[0] =
> > cpu_to_le32(stats.units_written);
> > +smart_ext.physical_media_units_read[0] = cpu_to_le32(stats.units_read);
> > +smart_ext.log_page_version = 0x0003;
> > +smart_ext.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
> > +smart_ext.log_page_uuid[1] = 0xAFD514C97C6F4F9C;
> > +
> > +if (!rae) {
> > +nvme_clear_events(n, NVME_AER_TYPE_SMART);
> > +}
> > +
> > +trans_len = MIN(sizeof(smart_ext) - off, buf_len);
> > +return nvme_c2h(n, (uint8_t *) &smart_ext + off, trans_len, req);
> > +}
> > +
> > static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> > uint64_t off, NvmeRequest *req)
> > {
> > @@ -4642,6 +4678,24 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n,
> > uint8_t csi, uint32_t buf_len,
> > return nvme_c2h(n, ((uint8_t *)&log) + off, trans_len, req);
> > }
> >
> > +static uint16_t nvme_vendor_specific_log(uint8_t lid, NvmeCtrl *n, uint8_t
> > rae,
> > + uint32_t buf_len, uint64_t off,
> > + NvmeRequest *req)
>
> `NvmeCtrl *n` must be first parameter.
Any reason why this is the case? I'll change it in my code, but would be
nice to understand the reason.
>
> > +{
> > +NvmeSubsystem *subsys = n->subsys;
> > +switch (lid) {
> > +case NVME_LOG_VENDOR_START:
>
> In this particular case, I think it is more clear if you simply use the
> hex value directly. The "meaning" of the log page id depends on if or
> not this is an controller implementing the OCP spec.
Agreed
>
> > +if (subsys->params.ocp) {
> > +return nvme_ocp_extended_smart_info(n, rae, buf_len, off,
> > req);
> > +}
> > +break;
> > +/* Add a case for each add
Re: [PATCH v2 3/3] nvme: Add physical writes/reads from OCP log
On Nov 14 14:50, Joel Granados wrote:
> In order to evaluate write amplification factor (WAF) within the storage
> stack it is important to know the number of bytes written to the
> controller. The existing SMART log value of Data Units Written is too
> coarse (given in units of 500 Kb) and so we add the SMART health
> information extended from the OCP specification (given in units of bytes).
>
> To accomodate different vendor specific specifications like OCP, we add a
> multiplexing function (nvme_vendor_specific_log) which will route to the
> different log functions based on arguments and log ids. We only return the
> OCP extended smart log when the command is 0xC0 and ocp has been turned on
> in the args.
>
> Though we add the whole nvme smart log extended structure, we only populate
> the physical_media_units_{read,written}, log_page_version and
> log_page_uuid.
>
> Signed-off-by: Joel Granados
>
> squash with main
>
> Signed-off-by: Joel Granados
Looks like you slightly messed up the squash ;)
Also, squash the previous patch (adding the ocp parameter) into this.
Please add a note in the documentation (docs/system/devices/nvme.rst)
about this parameter.
> ---
> hw/nvme/ctrl.c | 56
> include/block/nvme.h | 36
> 2 files changed, 92 insertions(+)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 220683201a..5e6a8150a2 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4455,6 +4455,42 @@ static void nvme_set_blk_stats(NvmeNamespace *ns,
> struct nvme_stats *stats)
> stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
> }
>
> +static uint16_t nvme_ocp_extended_smart_info(NvmeCtrl *n, uint8_t rae,
> + uint32_t buf_len, uint64_t off,
> + NvmeRequest *req)
> +{
> +NvmeNamespace *ns = NULL;
> +NvmeSmartLogExtended smart_ext = { 0 };
> +struct nvme_stats stats = { 0 };
> +uint32_t trans_len;
> +
> +if (off >= sizeof(smart_ext)) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +// Accumulate all stats from all namespaces
Use /* lower-case and no period */ for one sentence, one line comments.
I think scripts/checkpatch.pl picks this up.
> +for (int i = 1; i <= NVME_MAX_NAMESPACES; i++) {
> +ns = nvme_ns(n, i);
> +if (ns)
> +{
Paranthesis go on the same line as the `if`.
> +nvme_set_blk_stats(ns, &stats);
> +}
> +}
> +
> +smart_ext.physical_media_units_written[0] =
> cpu_to_le32(stats.units_written);
> +smart_ext.physical_media_units_read[0] = cpu_to_le32(stats.units_read);
> +smart_ext.log_page_version = 0x0003;
> +smart_ext.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
> +smart_ext.log_page_uuid[1] = 0xAFD514C97C6F4F9C;
> +
> +if (!rae) {
> +nvme_clear_events(n, NVME_AER_TYPE_SMART);
> +}
> +
> +trans_len = MIN(sizeof(smart_ext) - off, buf_len);
> +return nvme_c2h(n, (uint8_t *) &smart_ext + off, trans_len, req);
> +}
> +
> static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> uint64_t off, NvmeRequest *req)
> {
> @@ -4642,6 +4678,24 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t
> csi, uint32_t buf_len,
> return nvme_c2h(n, ((uint8_t *)&log) + off, trans_len, req);
> }
>
> +static uint16_t nvme_vendor_specific_log(uint8_t lid, NvmeCtrl *n, uint8_t
> rae,
> + uint32_t buf_len, uint64_t off,
> + NvmeRequest *req)
`NvmeCtrl *n` must be first parameter.
> +{
> +NvmeSubsystem *subsys = n->subsys;
> +switch (lid) {
> +case NVME_LOG_VENDOR_START:
In this particular case, I think it is more clear if you simply use the
hex value directly. The "meaning" of the log page id depends on if or
not this is an controller implementing the OCP spec.
> +if (subsys->params.ocp) {
> +return nvme_ocp_extended_smart_info(n, rae, buf_len, off,
> req);
> +}
> +break;
> +/* Add a case for each additional vendor specific log id */
Lower-case the comment.
> +}
> +
> +trace_pci_nvme_err_invalid_log_page(nvme_cid(req), lid);
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
> {
> NvmeCmd *cmd = &req->cmd;
> @@ -4683,6 +4737,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest
> *req)
> return nvme_error_info(n, rae, len, off, req);
> case NVME_LOG_SMART_INFO:
> return nvme_smart_info(n, rae, len, off, req);
> +case NVME_LOG_VENDOR_START...NVME_LOG_VENDOR_END:
> +return nvme_vendor_specific_log(lid, n, rae, len, off, req);
> case NVME_LOG_FW_SLOT_INFO:
> return nvme_fw_log_info(n, len, off, req);
> case NVME_LOG_CHANGED_NSLIS
