[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 13:58:15 +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=0AQ6SoYlc3WS+MVGsuSs3Udt9q2tHA2ImOQ9mn0ikFc=; b=B2IcZUUUTeqU0fjpAt/tLYwLQbGhUTy44sk8QwS1ffVyGOQtI2CPrrDWiOHlIq4pewCWG9TdBr9BS9qjo3R27+uIDp1tqcaFkMwAhujXeKEhBLt+VHkENb7wIrTZ3FnIkKxcuHbihJAtqJH+lI2QjJgMdxFs1AygiMLu3lKBSA/FZ6xWnfnvkG88DVJP7KtILCe6+aliuLUmB43ktLPmvZ51K3Xj7THnBWu+Gdt3DZnRZaiK3pjNtljeBptirdFKICvTc6CgJMVt+VaKUtKC4DVZMRr77UHgjGame4gSakb3zhWqZsS6SyhBWe4RxEeUvItTxDw7ej2sRfMxxhdB7A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=m74IX+Z/TzfzUxl/BVeTg40hEauobpOFKvcGQuLUPnQLfWhexJJp086GDtoh5p9yp9uW/Vi+iTfGaDhmAmQR7xYpYxhjIRT8UyfdzLGgRNn53+edUTNg8mBgyzc9nwpPH/UXou5psdbnQ9bQqpiFi7dDwk57Q0v33mjZ++dLTciki7lCecQQa2+80xgpRwPAu+uNslQbDwGbubxPxO21vAT2Ggdg8lv0YLgl0cv0DowC6/cdbM+ECNXUwrY8HUFa1hyIu2DHaAwm2U4TlF4JxSWKW0qr7LKy1EB/05gu37fUtaGaulD8Yhnv6o/wiafCaRo2jF1Wavh4KJB3oTd/TQ==
  • 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 11:58:47 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

As an aside, the suggested "bool wait" parameter would (right now) only
ever get passed a "true" argument, so I'm not convinced it's useful to
have at this point, as then we'd also need to deal with the "false"
case (requiring a completion interrupt to be arranged for, which we
have no handler for) despite that code path being unused (and hence
also un-testable).




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