On Jun 15 11:58, Jinhao Fan wrote: > > > On Jun 14, 2022, at 11:41 PM, Keith Busch <kbu...@kernel.org> wrote: > > > > It's a pretty nasty hack, and definitely not in compliance with the spec: > > the > > db_addr is supposed to be read-only from the device side, though I do think > > it's safe for this environment. Unless Klaus or anyone finds something I'm > > missing, I feel this is an acceptable compromise to address this odd > > discrepency. > > :) In my next patch I will check the performance numbers with this hack. Not > sure if updating db_addr value from the host will have any performance > implications but I guess it should be OK. >
I prefer we use the NVMe terminology to minimize misunderstandings, so "host" means the driver and "device" means the qemu side of things > > By the way, I noticed that the patch never updates the cq's ei_addr value. > > Is > > that on purpose? > > Klaus also raised a similar question in a prior comment. I think we need to > figure > this out before we move on to the v2 patch. I did this because the original > Google > extension patch did not update cq’s ei_addr. This seems to make sense because > the purpose of cq’s ei_addr is for the guest to notify the host about cq head > changes when necessary. However, the host does not need this notification > because we let the host proactively check for cq’s db_addr value when it wants > to post a new cqe. > This is also the only point where the host uses the cq’s > db_addr. Therefore, it is OK to postpone the check for cq’s db_addr to this > point, > instead of getting timely but not useful notifications by updating cq’s > ei_addr. > This helps to reduce the number of MMIO’s on the cq’s doorbell register. > True, it does reduce it, but it may leave CQEs "lingering" on the device side (since the device has not been notified that the host has consumed them). > Klaus, Keith, do you think this design makes sense? As I mentioned in my reply to John, I can see why this is a perfectly reasonable optimization, we don't really care about the lingering CQEs since we read the head anyway prior to posting completions. I jumped the gun here in my eagerness to be "spec compliant" ;)
signature.asc
Description: PGP signature