[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/9] AMD/IOMMU: re-work locking around sending of commands

  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 10 Jun 2021 14:53:12 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=X+UtJaeCpPZZSW2vKfmvlg34TfCyVyIQawQiaVpBwaQ=; b=Jgi7LeYXd6KOn8X2cWwGCSfyJf5wPkdspHAftzrYsHivVR0AlCAsyPyJAS/YwU1Jh0009zaywvtwPQ4J1izAgS/Ip0XoBOASSa2OkvyQiibk46MMcDd+6LlwU832xXlo1Cag46PtjXr0tDB3hpxPVEv12oL3lziFHrcKTs0DqDZB0SumUU7EBgXL5IaLgFCPvir050w1pBI/GLZvbhjhOQc5HPBBRX4eWF6GupSUXPZdMdSSlqncKkOssPkraPH/QJcDK+YZWV3233OIguzRFaxYfVpl3seXbBx15F5/VuBttWbN7KN02EE2wtIp3LwWP9Y34S07QuVg/x4YYZ2kDw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=l/O1qS2DSfWtwhCptCsmZwnEqi7L6K0yfBsT4w2crTGa8hAmCe+mK4RvYmWP7PoMOAyotBexK+QCqP5vqIGxEd04IwLrN3tqcnDZjmtx6T+kUIbjg3D5/3uafXXbd1pXLyABRZv4SKeOS+bs6MbrC2zcSqm45Htn0LWhvJ9DEgYsAzlDeqZX+rSRY5Npx9PaY0rGHmDzCwmILaLuBOMTCr0cmjBQ56IdofvY4YbVBHZE/fZvKws+Wfk0xTnJo62Wt+eci1Cexh5J4+sLTQkzJldLL76A1BW/0UK3UmXjkNILyFIb1ba0qUWEK2o/k73jpsIOMBcPTMol5EutqPtCXA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Paul Durrant <paul@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 10 Jun 2021 12:53:19 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 10.06.2021 13:58, Jan Beulich wrote:
> On 09.06.2021 12:53, Andrew Cooper wrote:
>> On 09/06/2021 10:27, Jan Beulich wrote:
>>> It appears unhelpful to me for flush_command_buffer() to block all
>>> progress elsewhere for the given IOMMU by holding its lock while
>>> waiting for command completion. Unless the lock is already held,
>>> acquire it in send_iommu_command(). Release it in all cases in
>>> flush_command_buffer(), before actually starting the wait loop.
>>> Some of the involved functions did/do get called with the lock already
>>> held: For amd_iommu_flush_intremap() we can simply move the locking
>>> inside. For amd_iommu_flush_device() and amd_iommu_flush_all_caches()
>>> the lock now gets dropped in the course of the function's operation.
>>> Where touching function headers anyway, also adjust types used to be
>>> better in line with our coding style and, where applicable, the
>>> functions' callers.
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> Reviewed-by: Paul Durrant <paul@xxxxxxx>
>> Honestly, I'm -2 to this.  It is horrible obfuscation of the logic.
>> I agree with the premise of not holding the lock when we don't need to,
>> but moving the lock/unlocks into different functions makes it impossible
>> to follow.  (Also, the static analysers are going to scream at this
>> patch, and rightfully so IMO.)
>> send_iommu_command() is static, as is flush_command_buffer(), so there
>> is no need to split the locking like this AFAICT.
>> Instead, each amd_iommu_flush_* external accessor knows exactly what it
>> is doing, and whether a wait descriptor is wanted. 
>> flush_command_buffer() wants merging into send_iommu_command() as a
>> "bool wait" parameter,
> A further remark on this particular suggestion: While this is likely
> doable, the result will presumably look a little odd: Besides the
> various code paths calling send_iommu_command() and then
> flush_command_buffer(), the former is also called _by_ the latter.
> I can give this a try, but I'd like to be halfway certain I won't
> be asked to undo that later on.
> And of course this won't help with the split locking, only with some
> of the passing around of the saved / to-be-restored eflags.

Actually, different observation: I don't think there really is a need
for either amd_iommu_flush_device() or amd_iommu_flush_all_caches()
to be called with the lock held. The callers can drop the lock, and
then all locking in iommu_cmd.c can likely be contained to
send_iommu_command() alone, without any need to fold in




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.