lkml.org 
[lkml]   [2021]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 0/9] iommu: Refactor flush queues into iommu-dma
From
On 2021-11-24 17:21, John Garry wrote:
> On 23/11/2021 14:10, Robin Murphy wrote:
>> As promised, this series cleans up the flush queue code and streamlines
>> it directly into iommu-dma. Since we no longer have per-driver DMA ops
>> implementations, a lot of the abstraction is now no longer necessary, so
>> there's a nice degree of simplification in the process. Un-abstracting
>> the queued page freeing mechanism is also the perfect opportunity to
>> revise which struct page fields we use so we can be better-behaved
>> from the MM point of view, thanks to Matthew.
>>
>> These changes should also make it viable to start using the gather
>> freelist in io-pgtable-arm, and eliminate some more synchronous
>> invalidations from the normal flow there, but that is proving to need a
>> bit more careful thought than I have time for in this cycle, so I've
>> parked that again for now and will revisit it in the new year.
>>
>> For convenience, branch at:
>>    https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/iova
>>
>> I've build-tested for x86_64, and boot-tested arm64 to the point of
>> confirming that put_pages_list() gets passed a valid empty list when
>> flushing, while everything else still works.
> My interest is in patches 2, 3, 7, 8, 9, and they look ok. I did a bit
> of testing for strict and non-strict mode on my arm64 system and no
> problems.
>
> Apart from this, I noticed that one possible optimization could be to
> avoid so many reads of fq_flush_finish_cnt, as we seem to have a pattern
> of fq_flush_iotlb()->atomic64_inc(fq_flush_finish_cnt) followed by a
> read of fq_flush_finish_cnt in fq_ring_free(), so we could use
> atomic64_inc_return(fq_flush_finish_cnt) and reuse the value. I think
> that any racing in fq_flush_finish_cnt accesses are latent, but maybe
> there is a flaw in this. However I tried something along these lines and
> got a 2.4% throughput gain for my storage scenario.

Yes, that sounds reasonable - off-hand I can't see that there's any more
potential for harmful races either. All that jumps out is the case where
the flush count gets bumped via queue_iova() while another CPU is
already running fq_flush_timeout(), where freeing newer IOVAs added
since the timeout is then more likely to be left to the local CPU, or
postponed until the next flush cycle entirely, rather than being piled
on to the guy already processing the for_each_possible_cpu() loop. And I
can't help thinking that could only be a *good* thing, given how the FQ
timeout seems to be a smoking gun in your "performance never recovers
after falling off the cliff" scenario :)

Robin.

\
 
 \ /
  Last update: 2021-11-24 19:34    [W:0.120 / U:1.668 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site