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. 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). Jan
