On Fri, 26 Apr 2024 17:39:57 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> I think this refers specifically to MSI-X, so use "MSI-X" in the
> subject.
ACK.
>
> On Thu, Apr 18, 2024 at 11:31:21AM -0400, Nirmal Patel wrote:
> > VMD MSI remapping is disabled by default for all the CPUs with 28c0
> > VMD deviceID. We used to disable remapping because drives supported
> > more vectors than the VMD so the performance was better without
> > remapping. Now with CPUs that support more than 64 (128 VMD MSIx
> > vectors for gen5) we no longer need to disable this feature.
>
> "because drives supported more vectors" ... I guess you are referring
> to typical devices that might be behind a VMD? But I assume there's
> no actual requirement that those devices be "drives", right?
Yes, I am referring to PCIe device with equal or more MSI-X vectors
than VMD. If a device has lesser than minimum VMD MSI-X count (64) than
VMD will not cause any performance issue while using MSI-X remapping.
>
> "CPUs that support more than 64 ... 128 VMD vectors" Are we talking
> about *CPUs* that support more vectors, or *VMDs* that support more
> vectors?
>
> I guess you probably think of CPUs here because VMD is integrated into
> the same package, right? That would explain the "CPUs with 28c0 VMD"
> comment. But the vmd driver doesn't care about that; it just claims a
> PCI device.
Yes, I will adjust my wordings; but I meant newer VMD which still has
same deviceID (28c0) as previous generations but it has more MSI-X
vectors.(i.e. 128)
>
> s/MSI remapping/MSI-X remapping/ (I think?)
> s/MSIx/MSI-X/ to match spec usage.
I will make adjustments.
>
> A reference to ee81ee84f873 ("PCI: vmd: Disable MSI-X remapping when
> possible"), which added VMD_FEAT_CAN_BYPASS_MSI_REMAP, might be
> useful because it has nice context.
>
> IIUC this will keep MSI-X remapping enabled in more cases, e.g., on
> new devices that support more vectors. What is the benefit of keeping
> it enabled?
Sorry I took longer to respond.
VMD MSI-X remapping was a performance bottleneck in certain
situations. Starting from 28c0, VMD has a capability to disable MSI-X
remapping and improve the I/O performance. The first iteration of 28c0
VMD HW had only 64 MSI-X vectors support while the newer iterations can
support up to 128 and VMD is no longer a bottleneck. So I thought it
would be a good idea to change it to MSI-X remapping default ON.
Also upon further testings, I noticed huge boost in performance because
of this CID patch:
https://lore.kernel.org/kvm/20240423174114.526704-5-jacob.jun.pan@xxxxxxxxxxxxxxx/T/
The performance boost we get from the CID patch as follow:
Kernel 6.8.8 : 1Drive: 2000, 4Drives: 2300
6.9.0-rc6 + CID + MSI-X remap Disable: 1Drive: 2700, 4Drives: 6010
6.9.0-rc6 + CID + MSI-X remap Enabled: 1Drive: 2700, 4Drives: 6100
Since there is no significant performance difference between MSI-X
enable and disable after addition of CID patch, I think we can drop this
patch for now until we see significant change in I/O performance due to
VMD's MSI-X remapping policy.
Thanks for your time.
-nirmal
>
> The ee81ee84f873 commit log suggests two issues:
>
> - Number of vectors available to child domain is limited by size of
> VMD MSI-X table.
>
> - Remapping means child interrupts have to go through the VMD domain
> interrupt handler instead of going straight to the device handler.
>
> But this commit log suggests that with more vectors, you can enable
> remapping even without a performance penalty? Maybe the VMD domain
> interrupt handler was only needed because of vector sharing?
>
> I'm just a little confused because this commit log doesn't say what
> the actual benefit is, other than "keeping remapping enabled", and I
> don't know enough to know why that's good.
>
> > Note, pci_msix_vec_count() failure is translated to ENODEV per
> > typical expectations that drivers may return ENODEV when some
> > driver-known fundamental detail of the device is missing.
> >
> > Signed-off-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>
> > ---
> > v1->v2: Updating commit message.
> > v2->v3: Use VMD MSI count instead of cpu count.
> > v3->v4: Updating commit message.
> > ---
> > drivers/pci/controller/vmd.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/pci/controller/vmd.c
> > b/drivers/pci/controller/vmd.c index 769eedeb8802..ba63af70bb63
> > 100644 --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -34,6 +34,8 @@
> > #define MB2_SHADOW_OFFSET 0x2000
> > #define MB2_SHADOW_SIZE 16
> >
> > +#define VMD_MIN_MSI_VECTOR_COUNT 64
> > +
> > enum vmd_features {
> > /*
> > * Device may contain registers which hint the physical
> > location of the @@ -807,13 +809,20 @@ static int
> > vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > sd->node = pcibus_to_node(vmd->dev->bus);
> >
> > + vmd->msix_count = pci_msix_vec_count(vmd->dev);
> > + if (vmd->msix_count < 0)
> > + return -ENODEV;
> > +
> > /*
> > * Currently MSI remapping must be enabled in guest
> > passthrough mode
> > * due to some missing interrupt remapping plumbing. This
> > is probably
> > * acceptable because the guest is usually CPU-limited and
> > MSI
> > * remapping doesn't become a performance bottleneck.
>
> This part of the comment might need some updating. I don't see the
> connection with guest passthrough mode in the code.
>
> > + * Disable MSI remapping only if supported by VMD hardware
> > and when
> > + * VMD MSI count is less than or equal to minimum MSI
> > count.
>
> Add blank line between paragraphs or rewrap into a single paragraph.
>
> > */
> > if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> > + vmd->msix_count > VMD_MIN_MSI_VECTOR_COUNT ||
> > offset[0] || offset[1]) {
>
> I think this conditional might be easier to read if it were inverted:
>
> if (features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) && ...) {
> vmd_set_msi_remapping(vmd, false);
> } else {
> ret = vmd_alloc_irqs(vmd);
> ...
> }
>
> Maybe a separate patch though.
>
> > ret = vmd_alloc_irqs(vmd);
> > if (ret)
> > --
> > 2.31.1
> >
