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

[Jinhao Fan]

> 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