Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <a8db0ed6-05f4-7c2d-c63e-5f2976d25a45@oracle.com>
Date:   Wed, 13 Dec 2023 09:09:18 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [RFC 2/2] PCI: acpiphp: slowdown hotplug if hotplugging multiple
 devices at a time
To:     "Michael S. Tsirkin" <mst@redhat.com>,
        Igor Mammedov <imammedo@redhat.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-pci@vger.kernel.org, lenb@kernel.org, bhelgaas@google.com,
        mika.westerberg@linux.intel.com, boris.ostrovsky@oracle.com,
        joe.jin@oracle.com, stable@vger.kernel.org,
        Fiona Ebner <f.ebner@proxmox.com>,
        Thomas Lamprecht <t.lamprecht@proxmox.com>
References: <20231213003614.1648343-1-imammedo@redhat.com>
 <20231213003614.1648343-3-imammedo@redhat.com>
 <CAJZ5v0gowV0WJd8pjwrDyHSJPvwgkCXYu9bDG7HHfcyzkSSY6w@mail.gmail.com>
 <CAMLWh55dr2e_R+TYVj=8cFfV==D-DfOZvAeq9JEehYs3nw6-OQ@mail.gmail.com>
 <20231213115248-mutt-send-email-mst@kernel.org>
Content-Language: en-US
From:   Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <20231213115248-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NGhPY0VscWVZOGl2WmNhVWwvNVZmVDRsdkZjV1ZiRSt1SW1FQlViMUxoZ3ZO?=
 =?utf-8?B?NCs2Q2Nld3A3THhiZkNrZ0FMaTVkRlV3SytiOXE4T0R3VkpDeU8vOXBVUlgy?=
 =?utf-8?B?aTFDcTk2cDdUbm9xaUpLSTU0THErbmV3SEI1aU9LeWtNd3lIaTFZWlJ4UFNC?=
 =?utf-8?B?clBjSGVGQzJDZVl0clhvZkFIb2tNOEx1alBZRHFGUmdqeVRpeVpBN0FxSlhk?=
 =?utf-8?B?MHV3S3ZpcVowS1JWRGRsYllPWWVBMUxLRjRicWhpTmNQUWRmUUFKMWVTSys3?=
 =?utf-8?B?T2FWREI5d0x3TStPK2xqOGRoenREaGI3U0JvNzFFS0RHOGZBWEg0RXB3UjBz?=
 =?utf-8?B?T2lzMVE4N0J3ZS9xL2E4TTNjWmc5aTFjSUxkV01sTzVlbWFFcFVqT21GaHZX?=
 =?utf-8?B?UjlvWHd1ZnVrL3p1OGdCdkhjMlJ4UjIvc1YxVTVTWTF5VGJQUnpZUHIrc1dC?=
 =?utf-8?B?Q3R2YXhBQkttMXBnbTRQSFdDM0I0VU15RmhoNC9wdVRsT05CQVN2dDd4cWxk?=
 =?utf-8?B?N0lCcFRBZ2JXaVV2Kzk2K3JYSkdoSVpRc1RuTlZPek11YjUwZDdjM1RkQ05u?=
 =?utf-8?B?ZWRvYngzQ2N4bWJtenhqVm9LS2pwaXhrY2VkUENYbDNxaW5JUFZENXZIckJw?=
 =?utf-8?B?d0FYRWE5RUtEb3ZWU2hGd25tK2h3MHUwTlZsYXhVZHBwcTZpTTRvVkE4MzhO?=
 =?utf-8?B?UmVESmVORG9aanh6alRwMVQ4dy8xa2lOeEZ0K2JMRmFYRUVHYlUwWWZqaDdV?=
 =?utf-8?B?Q3pISXQxbkxBZlJrQmk0M2hCK1AwR0hoQTkyZnpENWh0NVpZNk9OOW9rbnRo?=
 =?utf-8?B?MlRYSnQxWGU0WHVBb1JvODBXOWVqU1hhbUxWSnJQVVZacUZRa1dXVVcrREpu?=
 =?utf-8?B?U1dYUU5Jd0srZkJBRGs3dWVzcUYraXRpQlpvRXFzRjFYcjVIUVRVNmQ4cXpm?=
 =?utf-8?B?N1Q0czcxTTRGeFM3VVAzbWU0cHphb2RsMVZZNlU0aStLcExqd0d0WFdTcXND?=
 =?utf-8?B?c1Z2ZWJyMWdvYlVtYXFGT0hQN0hUbm1mQ3kxTHZrNGZTd0NYT3IwWWFnUlNO?=
 =?utf-8?B?eCtWUkFxTUlCKzhpa0lidUJmNm1UbVUwcEJoUWtESTN2SjJWTE5uSzhlMVNl?=
 =?utf-8?B?Y3FqVUROeVVoWEdMWmh4TFU1K1VSWVRvMzZsd0JVRHRxdUlSY1EzVWppaU90?=
 =?utf-8?B?UWVkRkdWTk9ZWmN3cW03MWl0bUZQVWhpSE1rVDV2QkIzaGE1Q3Y5cXZKZjdY?=
 =?utf-8?B?WlBNS3hQa1grM2ZNQnZ5ZE9XR3ZPLytEa2QwZkh6U0o4RzA3TlBUc2h3UlNJ?=
 =?utf-8?B?NTRKem5TakhRQjJtOGw5WWtqSFJoMkpGSy9yUjUySlR5TU9yd0FzaHZlWVZZ?=
 =?utf-8?B?Yll2bHVsM2pyUDBmcVR6NXR1TEJQSURZbXJHRXdvNFFrWkhFZytabnRNd29E?=
 =?utf-8?B?RE5YL2ltUGNPNENKOFRUNTJlOUd3SDJVbVJaSTRDL1dTTEQwQjIvOE1zZGxI?=
 =?utf-8?B?b3dGUjJMVWc5QXF6R3NRUEFEa05tR3VHc2MvSnhRNUw5bXgwbkRpUC92MWw0?=
 =?utf-8?B?S0JwNlVZNTllNkV1b0JyOC9Zd1h5c2NPNXpsK2tYTEw5cUgzT045b0w3OTVD?=
 =?utf-8?B?OE96T1RqUjc5WHc2ZmhvWjNSaFFFUlVvaEdWSkJteG4xRlV6SnF0TDdaS1dx?=
 =?utf-8?B?cDkyTEtndE8wT0VqWFJ5ZXJFdHR3SWhqUGFxaXdnK3RxM0NVaWRpbkQyTXBJ?=
 =?utf-8?B?d20zWFY2K01UcXRxdmZYUXNBbmlUWkJjRUxtanJnT2RLdjlmV1FFR3BlNnV6?=
 =?utf-8?B?WGwzZlo4M09MbTZ2eTBFOHVpM2x2S1VDWVlnNHM1WEUvclJHQnZQNnptT0dV?=
 =?utf-8?B?ZTJTMG9YSWlTQXJkR3dpUTgwSW9FSmRBQ21odWJwREFGSGYrN0ZZS3BtMmFz?=
 =?utf-8?B?R3NHL1BhenVkY1pxTFdWU1lNengyL2QycWdIeWtiUVVRZnpMTncreHNDRXp5?=
 =?utf-8?B?UEVTMjdZSFRlSnZtWWVQT3NRN2doZ21rTThIQnEwZ2NNOXpwUHhRN09sVG9Z?=
 =?utf-8?B?b1J3UUhYNHI3blZtZ09QdFVBbjJnem9xRWQrZFlyYkh4Z3RHQWJQbDloWWpk?=
 =?utf-8?Q?1JRQNaHxXpDW0gnSzQemzGXFU?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 5IGZmkYWXbVDhJqPsnVBMy+4xnkV1rwjpMU6Ja1lyIKX8sJ4jobgV/2Yi5h9h+SBy8I2r17UZsi/q9DsaCcA3W2yMSdB/3ZVv4iWtFOiWsVNBqWsSlW3kZo3b36oWRmpUrrNXVGatn1sGkZ2r/FeV/oUSDhwDmpVYDrrNpKSNpUuJhQuAi7v8jWsY/m75iHZGcbaOd5NHLjtxdjiC0uBINgOs9MEWRqvLWnb/2u66KJ7whvPGi8w27QUtcq6edqiTZEILbotsFPTzm/3OQLBMOvRgjK21DF4Z24GtQSjO+u6h1vLKDQqgK67d76pwf/C0HwKhO9rY4UbQPyma/dGuL5yF0nFk9swpBnmTEHJrWS9I8H5Jit3bxYdQjTr2d2DjSA+ntfjItqwVl53vgb0/PEURfGrTuYEa7FRCwn/W1XvDvzMqcOxPSu3lBv6vb4UATuOCy0aNz3xOlwXFUBDHfL6lmLFieYbGpnQHEv2mdbt7HtOQZqv6vs+hQSM1NYU8QGg1X/9ZitiGfhG1TwYIshHhL32BzYgwubrUTnlfgWfxoS1VISERXtt7kYhvsScbHAWYFNe4qsS5nQYpQ3Yla0i+mJpz7toQeeLcRNbImQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 834d3ab4-21d6-4a9a-53f5-08dbfbfe3e63
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 17:09:19.1632
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lXJhCI9GWBTbNPatw8PBvU1e1gvMhnpokZL0N/qTpb6pXXtToBAafsE3c8iJZ7kbBQ2by2Z3o5gKzOoLvXdVYg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5567
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-13_10,2023-12-13_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 spamscore=0
 malwarescore=0 mlxscore=0 mlxlogscore=999 adultscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312130122
X-Proofpoint-ORIG-GUID: txVwy_g0EtAzh7ZMwg-QBnFmc7OYX3eh
X-Proofpoint-GUID: txVwy_g0EtAzh7ZMwg-QBnFmc7OYX3eh
X-Spam-Status: No, score=-1.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 13 Dec 2023 09:10:06 -0800 (PST)

Hi Igor,

On 12/13/23 08:54, Michael S. Tsirkin wrote:
> On Wed, Dec 13, 2023 at 05:49:39PM +0100, Igor Mammedov wrote:
>> On Wed, Dec 13, 2023 at 2:08 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>
>>> On Wed, Dec 13, 2023 at 1:36 AM Igor Mammedov <imammedo@redhat.com> wrote:
>>>>
>>>> previous commit ("PCI: acpiphp: enable slot only if it hasn't been enabled already"
>>>> introduced a workaround to avoid a race between SCSI_SCAN_ASYNC job and
>>>> bridge reconfiguration in case of single HBA hotplug.
>>>> However in virt environment it's possible to pause machine hotplug several
>>>> HBAs and let machine run. That can hit the same race when 2nd hotplugged
>>>> HBA will start re-configuring bridge.
>>>> Do the same thing as SHPC and throttle down hotplug of 2nd and up
>>>> devices within single hotplug event.
>>>>
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>  drivers/pci/hotplug/acpiphp_glue.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>> index 6b11609927d6..30bca2086b24 100644
>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>> @@ -37,6 +37,7 @@
>>>>  #include <linux/mutex.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/acpi.h>
>>>> +#include <linux/delay.h>
>>>>
>>>>  #include "../pci.h"
>>>>  #include "acpiphp.h"
>>>> @@ -700,6 +701,7 @@ static void trim_stale_devices(struct pci_dev *dev)
>>>>  static void acpiphp_check_bridge(struct acpiphp_bridge *bridge)
>>>>  {
>>>>         struct acpiphp_slot *slot;
>>>> +        int nr_hp_slots = 0;
>>>>
>>>>         /* Bail out if the bridge is going away. */
>>>>         if (bridge->is_going_away)
>>>> @@ -723,6 +725,10 @@ static void acpiphp_check_bridge(struct acpiphp_bridge *bridge)
>>>>
>>>>                         /* configure all functions */
>>>>                         if (slot->flags != SLOT_ENABLED) {
>>>> +                               if (nr_hp_slots)
>>>> +                                       msleep(1000);
>>>
>>> Why is 1000 considered the most suitable number here?  Any chance to
>>> define a symbol for it?
>>
>> Timeout was borrowed from SHPC hotplug workflow where it apparently
>> makes race harder to reproduce.
>> (though it's not excuse to add more timeouts elsewhere)
>>
>>> And won't this affect the cases when the race in question is not a concern?
>>
>> In practice it's not likely, since even in virt scenario hypervisor won't
>> stop VM to hotplug device (which beats whole purpose of hotplug).
>>
>> But in case of a very slow VM (overcommit case) it's possible for
>> several HBA's to be hotplugged by the time acpiphp gets a chance
>> to handle the 1st hotplug event. SHPC is more or less 'safe' with its
>> 1sec delay.
>>
>>> Also, adding arbitrary timeouts is not the most robust way of
>>> addressing race conditions IMV.  Wouldn't it be better to add some
>>> proper synchronization between the pieces of code that can race with
>>> each other?
>>
>> I don't like it either, it's a stop gap measure to hide regression on
>> short notice,
>> which I can fixup without much risk in short time left, before folks
>> leave on holidays.
>> It's fine to drop the patch as chances of this happening are small.
>> [1/2] should cover reported cases.
>>
>> Since it's RFC, I basically ask for opinions on a proper way to fix
>> SCSI_ASYNC_SCAN
>> running wild while the hotplug is in progress (and maybe SCSI is not
>> the only user that
>> schedules async job from device probe).
> 
> Of course not. And things don't have to be scheduled from probe right?
> Can be triggered by an interrupt or userspace activity.

I agree with Michael. TBH, I am curious if the two patches can
workaround/resolve the issue.

Would you mind helping explain if to run enable_slot() for a new PCI device can
impact the other PCI devices existing on the bridge?

E.g.,:

1. Attach several virtio-scsi or virtio-net on the same bridge.

2. Trigger workload for those PCI devices. They may do mmio write to kick the
doorbell (to trigger KVM/QEMU ioeventfd) very frequently.

3. Now hot-add an extra PCI device. Since the slot is never enabled, it enables
the slot via enable_slot().

Can I assume the last enable_slot() will temporarily re-configure the bridge
window so that all other PCI devices' mmio will lose effect at that time point?

Since drivers always kick the doorbell conditionally, they may hang forever.

As I have reported, we used to have the similar issue.

PCI: Probe bridge window attributes once at enumeration-time
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51c48b310183ab6ba5419edfc6a8de889cc04521


Therefore, can I assume the issue is not because to re-enable an already-enabled
slot, but to touch the bridge window for more than once?

Thank you very much!

Dongli Zhang

> 
>> So adding synchronisation and testing
>> would take time (not something I'd do this late in the cycle).
>>
>> So far I'm thinking about adding rw mutex to bridge with the PCI
>> hotplug subsystem
>> being a writer while scsi scan jobs would be readers and wait till hotplug code
>> says it's safe to proceed.
>> I plan to work in this direction and give it some testing, unless
>> someone has a better idea.
> 
>>>
>>>> +
>>>> +                                ++nr_hp_slots;
>>>>                                 enable_slot(slot, true);
>>>>                         }
>>>>                 } else {
>>>> --
>>>
>