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;
Message-ID: <0603c75d-82d3-01d5-ffe7-b648c1f02f0e@linux.intel.com>
Date:   Wed, 29 Mar 2023 23:49:03 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v4] PCI: vmd: Add the module param to adjust MSI mode
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>,
        Xinghui Li <korantwork@gmail.com>
Cc:     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>
References: <20230329163107.GA3061927@bhelgaas>
From:   "Patel, Nirmal" <nirmal.patel@linux.intel.com>
In-Reply-To: <20230329163107.GA3061927@bhelgaas>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/29/2023 9:31 AM, Bjorn Helgaas wrote:
> 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

How about adding a boolean flag by comparing user input for module
parameter msi_remap? and add the flag at

    - if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) || msi_flag
        || offset[0] || offset[1])

Correct if I am wrong, but in this way we can cover all the cases.
If user adds msi_remap=on, msi_flag=true and enables remapping.
If user adds msi_remap=off, msi_flag=false and disables remapping.
If user doesn't add anything, msi_flag=false and decision will be
made same as current implementation. This will cover guest OS case
as well.