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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/9] AMD/IOMMU: re-work locking around sending of commands
Date: Thu, 10 Jun 2021 14:53:12 +0200	[thread overview]
Message-ID: <f3f47e98-e18c-50f1-ade9-e4b4df055b5a@suse.com> (raw)
In-Reply-To: <11f4cb58-d0f4-f735-2b18-5e02cb6950e4@suse.com>

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@suse.com>
>>> Reviewed-by: Paul Durrant <paul@xen.org>
>>
>> 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
flush_command_buffer().

Jan