mbox series

[v4,0/2] Add OCP extended log to nvme QEMU

Message ID 20221125094808.1856024-1-j.granados@samsung.com (mailing list archive)
Headers show
Series Add OCP extended log to nvme QEMU | expand

Message

Joel Granados Nov. 25, 2022, 9:48 a.m. UTC
The motivation and description are contained in the last patch in this set.
Will copy paste it here for convenience:

    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 accommodate 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.

V4 changes:
1. Fixed cpu_to_le64 instead of cpu_to_le32
2. Variable naming : uuid -> guid
3. Changed how the guid value appears in the code:
   Used to be:
        smart_l.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
        smart_l.log_page_uuid[1] = 0xAFD514C97C6F4F9C;

   Now is:
        static const uint8_t guid[16] = {
            0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
            0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
        };

   This is different from what @klaus suggested because I want to keep it
   consistent to what nvme-cli currently implements. I think here we can
   either change both nvme-cli and this patch or leave the order of the
   bytes as they are here. This all depends on how you interpret the Spec
   (which is ambiguous)

V3 changes:
1. Corrected a bunch of checkpatch issues. Since I changed the first patch
   I did not include the reviewed-by.
2. Included some documentation in nvme.rst for the ocp argument
3. Squashed the ocp arg changes into the main patch.
4. Fixed several comments and an open parenthesis
5. Hex values are now in lower case.
6. Change the reserved format to rsvd<BYTEOFFSET>
7. Made sure that NvmeCtrl is the first arg in all the functions.
8. Fixed comment on commit of main patch

V2 changes:
1. I moved the ocp parameter from the namespace to the subsystem as it is
   defined there in the OCP specification
2. I now accumulate statistics from all namespaces and report them back on
   the extended log as per the spec.
3. I removed the default case in the switch in nvme_vendor_specific_log as
   it does not have any special function.

Joel Granados (2):
  nvme: Move adjustment of data_units{read,written}
  nvme: Add physical writes/reads from OCP log

 docs/system/devices/nvme.rst |  7 ++++
 hw/nvme/ctrl.c               | 73 +++++++++++++++++++++++++++++++++---
 hw/nvme/nvme.h               |  1 +
 include/block/nvme.h         | 36 ++++++++++++++++++
 4 files changed, 111 insertions(+), 6 deletions(-)

Comments

Joel Granados Dec. 8, 2022, 4:09 p.m. UTC | #1
ping.

Is the solution to the guid constant ok?

Best

On Fri, Nov 25, 2022 at 10:48:06AM +0100, Joel Granados wrote:
> The motivation and description are contained in the last patch in this set.
> Will copy paste it here for convenience:
> 
>     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 accommodate 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.
> 
> V4 changes:
> 1. Fixed cpu_to_le64 instead of cpu_to_le32
> 2. Variable naming : uuid -> guid
> 3. Changed how the guid value appears in the code:
>    Used to be:
>         smart_l.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
>         smart_l.log_page_uuid[1] = 0xAFD514C97C6F4F9C;
> 
>    Now is:
>         static const uint8_t guid[16] = {
>             0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
>             0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
>         };
> 
>    This is different from what @klaus suggested because I want to keep it
>    consistent to what nvme-cli currently implements. I think here we can
>    either change both nvme-cli and this patch or leave the order of the
>    bytes as they are here. This all depends on how you interpret the Spec
>    (which is ambiguous)
> 
> V3 changes:
> 1. Corrected a bunch of checkpatch issues. Since I changed the first patch
>    I did not include the reviewed-by.
> 2. Included some documentation in nvme.rst for the ocp argument
> 3. Squashed the ocp arg changes into the main patch.
> 4. Fixed several comments and an open parenthesis
> 5. Hex values are now in lower case.
> 6. Change the reserved format to rsvd<BYTEOFFSET>
> 7. Made sure that NvmeCtrl is the first arg in all the functions.
> 8. Fixed comment on commit of main patch
> 
> V2 changes:
> 1. I moved the ocp parameter from the namespace to the subsystem as it is
>    defined there in the OCP specification
> 2. I now accumulate statistics from all namespaces and report them back on
>    the extended log as per the spec.
> 3. I removed the default case in the switch in nvme_vendor_specific_log as
>    it does not have any special function.
> 
> Joel Granados (2):
>   nvme: Move adjustment of data_units{read,written}
>   nvme: Add physical writes/reads from OCP log
> 
>  docs/system/devices/nvme.rst |  7 ++++
>  hw/nvme/ctrl.c               | 73 +++++++++++++++++++++++++++++++++---
>  hw/nvme/nvme.h               |  1 +
>  include/block/nvme.h         | 36 ++++++++++++++++++
>  4 files changed, 111 insertions(+), 6 deletions(-)
> 
> -- 
> 2.30.2
>
Klaus Jensen Dec. 13, 2022, 8:11 a.m. UTC | #2
On Nov 25 10:48, Joel Granados wrote:
> The motivation and description are contained in the last patch in this set.
> Will copy paste it here for convenience:
> 
>     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 accommodate 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.
> 
> V4 changes:
> 1. Fixed cpu_to_le64 instead of cpu_to_le32
> 2. Variable naming : uuid -> guid
> 3. Changed how the guid value appears in the code:
>    Used to be:
>         smart_l.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
>         smart_l.log_page_uuid[1] = 0xAFD514C97C6F4F9C;
> 
>    Now is:
>         static const uint8_t guid[16] = {
>             0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
>             0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
>         };
> 
>    This is different from what @klaus suggested because I want to keep it
>    consistent to what nvme-cli currently implements. I think here we can
>    either change both nvme-cli and this patch or leave the order of the
>    bytes as they are here. This all depends on how you interpret the Spec
>    (which is ambiguous)
> 
> V3 changes:
> 1. Corrected a bunch of checkpatch issues. Since I changed the first patch
>    I did not include the reviewed-by.
> 2. Included some documentation in nvme.rst for the ocp argument
> 3. Squashed the ocp arg changes into the main patch.
> 4. Fixed several comments and an open parenthesis
> 5. Hex values are now in lower case.
> 6. Change the reserved format to rsvd<BYTEOFFSET>
> 7. Made sure that NvmeCtrl is the first arg in all the functions.
> 8. Fixed comment on commit of main patch
> 
> V2 changes:
> 1. I moved the ocp parameter from the namespace to the subsystem as it is
>    defined there in the OCP specification
> 2. I now accumulate statistics from all namespaces and report them back on
>    the extended log as per the spec.
> 3. I removed the default case in the switch in nvme_vendor_specific_log as
>    it does not have any special function.
> 
> Joel Granados (2):
>   nvme: Move adjustment of data_units{read,written}
>   nvme: Add physical writes/reads from OCP log
> 
>  docs/system/devices/nvme.rst |  7 ++++
>  hw/nvme/ctrl.c               | 73 +++++++++++++++++++++++++++++++++---
>  hw/nvme/nvme.h               |  1 +
>  include/block/nvme.h         | 36 ++++++++++++++++++
>  4 files changed, 111 insertions(+), 6 deletions(-)
> 
> -- 
> 2.30.2
> 

LGTM,

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>