Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Mar 2023 11:31:07 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Xinghui Li <korantwork@gmail.com>
Cc:     nirmal.patel@linux.intel.com, kbusch@kernel.org,
        jonathan.derrick@linux.dev, lpieralisi@kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        Xinghui Li <korantli@tencent.com>
Subject: Re: [PATCH v4] PCI: vmd: Add the module param to adjust MSI mode
Message-ID: <20230329163107.GA3061927@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEm4hYW3zvte_kdWMFrv-pKVijQaL7KzbE12WHuO14s73ExUbQ@mail.gmail.com>
Precedence: bulk

On Wed, Mar 29, 2023 at 04:57:08PM +0800, Xinghui Li wrote:
> On Wed, Mar 29, 2023 at 5:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > It would also be nice to include a hint about why a user would choose
> > "on" or "off".  What is the performance effect?  What sort of I/O
> > scenario would lead you to choose "on" vs "off"?
> >
> Before this patch, I sent the patch named :
> PCI: vmd: Do not disable MSI-X remapping in VMD 28C0 controller
> (patchwork link:
> https://patchwork.kernel.org/project/linux-pci/patch/20221222072603.1175248-1-korantwork@gmail.com/)
> We found the 4k rand read's iops could drop 50% if 4 NVMEs were
> mounted in one PCIE port with VMD MSI bypass.
> I suppose this is because the VMD Controller can aggregate interrupts.
> But those test result is so long that I didn't add them to this patch
> commit log.
> If you believe it is necessary, I will try to add some simple instructions

I don't think we need detailed performance numbers, but we need
something like:

  - "msi_remap=off" improves interrupt handling performance by
    avoiding the VMD MSI-X domain interrupt handler

  - But "msi_remap=on" is needed when ...?

> > ee81ee84f873 ("PCI: vmd: Disable MSI-X remapping when possible")
> > suggests that MSI-X remapping (I assume the "msi_remap=on" case):
> >
> >   - Limits the number MSI-X vectors available to child devices to the
> >     number of VMD MSI-X vectors.
> >
> >   - Reduces interrupt handling performance because child device
> >     interrupts have to go through the VMD MSI-X domain interrupt
> >     handler.
> >
> > So I assume "msi_remap=off" would remove that MSI-X vector limit and
> > improve interrupt handling performance?
> >
> > But obviously there's more to consider because those are both good
> > things and if we could do that all the time, we would.  So there must
> > be cases where we *have* to remap.  ee81ee84f873 suggests that not all
> > VMD devices support disabling remap.  There's also a hint that some
> > virt configs require it.
> >
> I used to just want to disable 28C0's VMD MSI bypass by default.
> But Nirmal suggested the current method by adjusting the param.
> Because he and other reviewers worry there are some other scenarios we
> didn't consider.
> Adding a method to adjust VMD'S MSI-X mode is better.

This commit log doesn't outline any of those other scenarios, and it
doesn't say anything about when "msi_remap=on" or "msi_remap=off"
would be necessary or desired, so I have no idea how users are
supposed to figure out whether or not to use this parameter.

> > This patch doesn't enforce either of those things.  What happens if
> > the user gets it wrong?
>
> If I am wrong, please feel free to correct me at any time.
> I place the "vmd_config_msi_remap_param" that is VMD MSI-X's mode
> param configuring helper front
> "vmd_enable_domain". So, It will not change the logic disabling
> remapping from ee81ee84f873, such as
> "Currently MSI remapping must be enabled in guest passthrough mode".
> So, if the user config the wrong type, it will not work, and they can
> find it by dmesg.

That's kind of a problem.  I'm not in favor of something failing and
the user having to debug it via dmesg.  That causes user frustration
and problem reports.

I don't know what "guest passthrough mode" is.  Can you detect that
automatically?

Bjorn