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 <jbeul...@suse.com> >>> Reviewed-by: Paul Durrant <p...@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
