[PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support

Jinhao Fan posted 2 patches 1 year, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220616123408.3306055-1-fanjinhao21s@ict.ac.cn
Maintainers: Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
hw/nvme/ctrl.c       | 118 ++++++++++++++++++++++++++++++++++++++++++-
hw/nvme/nvme.h       |   8 +++
hw/nvme/trace-events |   5 ++
include/block/nvme.h |   2 +
4 files changed, 132 insertions(+), 1 deletion(-)
[PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Jinhao Fan 1 year, 11 months ago
This patch adds shadow doorbell buffer support in NVMe 1.3 to QEMU
NVMe. The Doorbell Buffer Config admin command is implemented for the
guest to enable shadow doobell buffer. When this feature is enabled, each
SQ/CQ is associated with two buffers, i.e., Shadow Doorbell buffer and
EventIdx buffer. According to the Spec, each queue's doorbell register
is only updated when the Shadow Doorbell buffer value changes from being
less than or equal to the value of the corresponding EventIdx buffer
entry to being greater than that value. Therefore, the number of MMIO's
on the doorbell registers is greatly reduced.

This patch is adapted from Huaicheng Li's patch[1] in 2018.

[1] https://patchwork.kernel.org/project/qemu-devel/patch/20180305194906.GA3630@gmail.com/

IOPS comparison with FIO:

iodepth    1      2      4      8
  QEMU   25.1k  25.9k  24.5k  24.0k
 +dbbuf  29.1k  60.1k  99.8k  82.5k

MMIO's per IO measured by perf-kvm:

iodepth    1      2      4      8
  QEMU   2.01   1.99   1.99   1.99
 +dbbuf  1.00   0.52   0.27   0.46

The tests are done on Ubuntu 22.04 with 5.15.0-33 kernel with Intel(R) 
Xeon(R) Gold 6248R CPU @ 3.00GHz.

QEMU set up:

bin/x86_64-softmmu/qemu-system-x86_64 \
    -name "nvme-test" \
    -machine accel=kvm \
    -cpu host \
    -smp 4 \
    -m 8G \
    -daemonize \
    -device virtio-scsi-pci,id=scsi0 \
    -device scsi-hd,drive=hd0 \
    -drive file=$OSIMGF,if=none,aio=native,cache=none,format=qcow2,id=hd0,snapshot=on \
    -drive "id=nvm,if=none,file=null-co://,file.read-zeroes=on,format=raw" \
    -device nvme,serial=deadbeef,drive=nvm \
    -net user,hostfwd=tcp::8080-:22 \
    -net nic,model=virtio

FIO configuration:

[global]
ioengine=libaio
filename=/dev/nvme0n1
thread=1
group_reporting=1
direct=1
verify=0
time_based=1
ramp_time=0
runtime=30
;size=1G
;iodepth=1
rw=randread
bs=4k

[test]
numjobs=1

Changes since v2:
  - Do not ignore admin queue updates in nvme_process_db and nvme_post_cqes
  - Calculate db_addr and ei_addr in hard-coded way

Changes since v1:
  - Add compatibility with hosts that do not use admin queue shadow doorbell

Jinhao Fan (2):
  hw/nvme: Implement shadow doorbell buffer support
  hw/nvme: Add trace events for shadow doorbell buffer

 hw/nvme/ctrl.c       | 118 ++++++++++++++++++++++++++++++++++++++++++-
 hw/nvme/nvme.h       |   8 +++
 hw/nvme/trace-events |   5 ++
 include/block/nvme.h |   2 +
 4 files changed, 132 insertions(+), 1 deletion(-)

-- 
2.25.1
Re: [PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Klaus Jensen 1 year, 10 months ago
On Jun 16 20:34, Jinhao Fan wrote:
> This patch adds shadow doorbell buffer support in NVMe 1.3 to QEMU
> NVMe. The Doorbell Buffer Config admin command is implemented for the
> guest to enable shadow doobell buffer. When this feature is enabled, each
> SQ/CQ is associated with two buffers, i.e., Shadow Doorbell buffer and
> EventIdx buffer. According to the Spec, each queue's doorbell register
> is only updated when the Shadow Doorbell buffer value changes from being
> less than or equal to the value of the corresponding EventIdx buffer
> entry to being greater than that value. Therefore, the number of MMIO's
> on the doorbell registers is greatly reduced.
> 
> This patch is adapted from Huaicheng Li's patch[1] in 2018.
> 
> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20180305194906.GA3630@gmail.com/
> 
> IOPS comparison with FIO:
> 
> iodepth    1      2      4      8
>   QEMU   25.1k  25.9k  24.5k  24.0k
>  +dbbuf  29.1k  60.1k  99.8k  82.5k
> 
> MMIO's per IO measured by perf-kvm:
> 
> iodepth    1      2      4      8
>   QEMU   2.01   1.99   1.99   1.99
>  +dbbuf  1.00   0.52   0.27   0.46
> 
> The tests are done on Ubuntu 22.04 with 5.15.0-33 kernel with Intel(R) 
> Xeon(R) Gold 6248R CPU @ 3.00GHz.
> 
> QEMU set up:
> 
> bin/x86_64-softmmu/qemu-system-x86_64 \
>     -name "nvme-test" \
>     -machine accel=kvm \
>     -cpu host \
>     -smp 4 \
>     -m 8G \
>     -daemonize \
>     -device virtio-scsi-pci,id=scsi0 \
>     -device scsi-hd,drive=hd0 \
>     -drive file=$OSIMGF,if=none,aio=native,cache=none,format=qcow2,id=hd0,snapshot=on \
>     -drive "id=nvm,if=none,file=null-co://,file.read-zeroes=on,format=raw" \
>     -device nvme,serial=deadbeef,drive=nvm \
>     -net user,hostfwd=tcp::8080-:22 \
>     -net nic,model=virtio
> 
> FIO configuration:
> 
> [global]
> ioengine=libaio
> filename=/dev/nvme0n1
> thread=1
> group_reporting=1
> direct=1
> verify=0
> time_based=1
> ramp_time=0
> runtime=30
> ;size=1G
> ;iodepth=1
> rw=randread
> bs=4k
> 
> [test]
> numjobs=1
> 
> Changes since v2:
>   - Do not ignore admin queue updates in nvme_process_db and nvme_post_cqes
>   - Calculate db_addr and ei_addr in hard-coded way
> 
> Changes since v1:
>   - Add compatibility with hosts that do not use admin queue shadow doorbell
> 
> Jinhao Fan (2):
>   hw/nvme: Implement shadow doorbell buffer support
>   hw/nvme: Add trace events for shadow doorbell buffer
> 
>  hw/nvme/ctrl.c       | 118 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/nvme/nvme.h       |   8 +++
>  hw/nvme/trace-events |   5 ++
>  include/block/nvme.h |   2 +
>  4 files changed, 132 insertions(+), 1 deletion(-)
> 
> -- 
> 2.25.1
> 
> 

Jinhao,

Thanks, applied to nvme-next!
Re: [PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Keith Busch 1 year, 11 months ago
On Thu, Jun 16, 2022 at 08:34:06PM +0800, Jinhao Fan wrote:
> This patch adds shadow doorbell buffer support in NVMe 1.3 to QEMU
> NVMe. The Doorbell Buffer Config admin command is implemented for the
> guest to enable shadow doobell buffer. When this feature is enabled, each
> SQ/CQ is associated with two buffers, i.e., Shadow Doorbell buffer and
> EventIdx buffer. According to the Spec, each queue's doorbell register
> is only updated when the Shadow Doorbell buffer value changes from being
> less than or equal to the value of the corresponding EventIdx buffer
> entry to being greater than that value. Therefore, the number of MMIO's
> on the doorbell registers is greatly reduced.

Looks good to me, and passes my sanity tests.

Reviewed-by: Keith Busch <kbusch@kernel.org>
Re: [PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Klaus Jensen 1 year, 11 months ago
On Jun 16 20:34, Jinhao Fan wrote:
> This patch adds shadow doorbell buffer support in NVMe 1.3 to QEMU
> NVMe. The Doorbell Buffer Config admin command is implemented for the
> guest to enable shadow doobell buffer. When this feature is enabled, each
> SQ/CQ is associated with two buffers, i.e., Shadow Doorbell buffer and
> EventIdx buffer. According to the Spec, each queue's doorbell register
> is only updated when the Shadow Doorbell buffer value changes from being
> less than or equal to the value of the corresponding EventIdx buffer
> entry to being greater than that value. Therefore, the number of MMIO's
> on the doorbell registers is greatly reduced.
> 
> This patch is adapted from Huaicheng Li's patch[1] in 2018.
> 
> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20180305194906.GA3630@gmail.com/
> 
> IOPS comparison with FIO:
> 
> iodepth    1      2      4      8
>   QEMU   25.1k  25.9k  24.5k  24.0k
>  +dbbuf  29.1k  60.1k  99.8k  82.5k
> 
> MMIO's per IO measured by perf-kvm:
> 
> iodepth    1      2      4      8
>   QEMU   2.01   1.99   1.99   1.99
>  +dbbuf  1.00   0.52   0.27   0.46
> 
> The tests are done on Ubuntu 22.04 with 5.15.0-33 kernel with Intel(R) 
> Xeon(R) Gold 6248R CPU @ 3.00GHz.
> 
> QEMU set up:
> 
> bin/x86_64-softmmu/qemu-system-x86_64 \
>     -name "nvme-test" \
>     -machine accel=kvm \
>     -cpu host \
>     -smp 4 \
>     -m 8G \
>     -daemonize \
>     -device virtio-scsi-pci,id=scsi0 \
>     -device scsi-hd,drive=hd0 \
>     -drive file=$OSIMGF,if=none,aio=native,cache=none,format=qcow2,id=hd0,snapshot=on \
>     -drive "id=nvm,if=none,file=null-co://,file.read-zeroes=on,format=raw" \
>     -device nvme,serial=deadbeef,drive=nvm \
>     -net user,hostfwd=tcp::8080-:22 \
>     -net nic,model=virtio
> 
> FIO configuration:
> 
> [global]
> ioengine=libaio
> filename=/dev/nvme0n1
> thread=1
> group_reporting=1
> direct=1
> verify=0
> time_based=1
> ramp_time=0
> runtime=30
> ;size=1G
> ;iodepth=1
> rw=randread
> bs=4k
> 
> [test]
> numjobs=1
> 
> Changes since v2:
>   - Do not ignore admin queue updates in nvme_process_db and nvme_post_cqes
>   - Calculate db_addr and ei_addr in hard-coded way
> 
> Changes since v1:
>   - Add compatibility with hosts that do not use admin queue shadow doorbell
> 
> Jinhao Fan (2):
>   hw/nvme: Implement shadow doorbell buffer support
>   hw/nvme: Add trace events for shadow doorbell buffer
> 
>  hw/nvme/ctrl.c       | 118 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/nvme/nvme.h       |   8 +++
>  hw/nvme/trace-events |   5 ++
>  include/block/nvme.h |   2 +
>  4 files changed, 132 insertions(+), 1 deletion(-)
> 
> -- 
> 2.25.1
> 

LGTM,

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
Re: [PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Jinhao Fan 1 year, 11 months ago

> On Jun 17, 2022, at 7:54 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> 
> On Jun 16 20:34, Jinhao Fan wrote:
>> This patch adds shadow doorbell buffer support in NVMe 1.3 to QEMU
>> NVMe. The Doorbell Buffer Config admin command is implemented for the
>> guest to enable shadow doobell buffer. When this feature is enabled, each
>> SQ/CQ is associated with two buffers, i.e., Shadow Doorbell buffer and
>> EventIdx buffer. According to the Spec, each queue's doorbell register
>> is only updated when the Shadow Doorbell buffer value changes from being
>> less than or equal to the value of the corresponding EventIdx buffer
>> entry to being greater than that value. Therefore, the number of MMIO's
>> on the doorbell registers is greatly reduced.
>> 
>> This patch is adapted from Huaicheng Li's patch[1] in 2018.
>> 
>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20180305194906.GA3630@gmail.com/
>> 
>> IOPS comparison with FIO:
>> 
>> iodepth    1      2      4      8
>>  QEMU   25.1k  25.9k  24.5k  24.0k
>> +dbbuf  29.1k  60.1k  99.8k  82.5k
>> 
>> MMIO's per IO measured by perf-kvm:
>> 
>> iodepth    1      2      4      8
>>  QEMU   2.01   1.99   1.99   1.99
>> +dbbuf  1.00   0.52   0.27   0.46
>> 
>> The tests are done on Ubuntu 22.04 with 5.15.0-33 kernel with Intel(R) 
>> Xeon(R) Gold 6248R CPU @ 3.00GHz.
>> 
>> QEMU set up:
>> 
>> bin/x86_64-softmmu/qemu-system-x86_64 \
>>    -name "nvme-test" \
>>    -machine accel=kvm \
>>    -cpu host \
>>    -smp 4 \
>>    -m 8G \
>>    -daemonize \
>>    -device virtio-scsi-pci,id=scsi0 \
>>    -device scsi-hd,drive=hd0 \
>>    -drive file=$OSIMGF,if=none,aio=native,cache=none,format=qcow2,id=hd0,snapshot=on \
>>    -drive "id=nvm,if=none,file=null-co://,file.read-zeroes=on,format=raw" \
>>    -device nvme,serial=deadbeef,drive=nvm \
>>    -net user,hostfwd=tcp::8080-:22 \
>>    -net nic,model=virtio
>> 
>> FIO configuration:
>> 
>> [global]
>> ioengine=libaio
>> filename=/dev/nvme0n1
>> thread=1
>> group_reporting=1
>> direct=1
>> verify=0
>> time_based=1
>> ramp_time=0
>> runtime=30
>> ;size=1G
>> ;iodepth=1
>> rw=randread
>> bs=4k
>> 
>> [test]
>> numjobs=1
>> 
>> Changes since v2:
>>  - Do not ignore admin queue updates in nvme_process_db and nvme_post_cqes
>>  - Calculate db_addr and ei_addr in hard-coded way
>> 
>> Changes since v1:
>>  - Add compatibility with hosts that do not use admin queue shadow doorbell
>> 
>> Jinhao Fan (2):
>>  hw/nvme: Implement shadow doorbell buffer support
>>  hw/nvme: Add trace events for shadow doorbell buffer
>> 
>> hw/nvme/ctrl.c       | 118 ++++++++++++++++++++++++++++++++++++++++++-
>> hw/nvme/nvme.h       |   8 +++
>> hw/nvme/trace-events |   5 ++
>> include/block/nvme.h |   2 +
>> 4 files changed, 132 insertions(+), 1 deletion(-)
>> 
>> -- 
>> 2.25.1
>> 
> 
> LGTM,
> 
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> 

Great!

I have two questions:

How many “Reviewed-by”’s do I need to get my patch applied?

Do I need to post a v4 patch to add the “Reviewed-by”’s in my commit 
message?

Thanks,
Jinhao Fan
Re: [PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Klaus Jensen 1 year, 11 months ago
On Jun 17 20:47, Jinhao Fan wrote:
> 
> 
> > On Jun 17, 2022, at 7:54 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> > 
> > LGTM,
> > 
> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> > 
> 
> Great!
> 
> I have two questions:
> 
> How many “Reviewed-by”’s do I need to get my patch applied?
> 

That depends ;) The maintainers decide that.

> Do I need to post a v4 patch to add the “Reviewed-by”’s in my commit 
> message?
> 

Nope, the maintainer will pick that up when applying.
Re: [PATCH v3 0/2] hw/nvme: Add shadow doorbell buffer support
Posted by Jinhao Fan 1 year, 11 months ago
> On Jun 17, 2022, at 8:56 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> 
> On Jun 17 20:47, Jinhao Fan wrote:
>> 
>> 
>>> On Jun 17, 2022, at 7:54 PM, Klaus Jensen <its@irrelevant.dk> wrote:
>>> 
>>> LGTM,
>>> 
>>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>>> 
>> 
>> Great!
>> 
>> I have two questions:
>> 
>> How many “Reviewed-by”’s do I need to get my patch applied?
>> 
> 
> That depends ;) The maintainers decide that.
> 
>> Do I need to post a v4 patch to add the “Reviewed-by”’s in my commit 
>> message?
>> 
> 
> Nope, the maintainer will pick that up when applying.

Gotcha! Thanks!