Virtualization - Mar 2021 - [PATCH v5 08/11] vduse: Implement an MMU-based IOMMU driver

? 2021/3/26 ??1:14, Yongji Xie ??:
>>>>>>> +     }
>>>>>>> +     map->bounce_page = page;
>>>>>>> +
>>>>>>> +     /* paired with vduse_domain_map_page() */
>>>>>>> +     smp_mb();
>>>>>> So this is suspicious. It's better to explain like,
we need make sure A
>>>>>> must be done after B.
>>>>> OK. I see. It's used to protect this pattern:
>>>>>
>>>>>       vduse_domain_alloc_bounce_page:         
vduse_domain_map_page:
>>>>>       write map->bounce_page                          
write map->orig_phys
>>>>>       mb()                                                 
mb()
>>>>>       read map->orig_phys                               
read map->bounce_page
>>>>>
>>>>> Make sure there will always be a path to do bouncing.
>>>> Ok.
>>>>
>>>>
>>>>>> And it looks to me the iotlb_lock is sufficnet to do
the synchronization
>>>>>> here. E.g any reason that you don't take it in
>>>>>> vduse_domain_map_bounce_page().
>>>>>>
>>>>> Yes, we can. But the performance in multi-queue cases will
go down if
>>>>> we use iotlb_lock on this critical path.
>>>>>
>>>>>> And what's more, is there anyway to aovid holding
the spinlock during
>>>>>> bouncing?
>>>>>>
>>>>> Looks like we can't. In the case that multiple page
faults happen on
>>>>> the same page, we should make sure the bouncing is done
before any
>>>>> page fault handler returns.
>>>> So it looks to me all those extra complexitiy comes from the
fact that
>>>> the bounce_page and orig_phys are set by different places so we
need to
>>>> do the bouncing in two places.
>>>>
>>>> I wonder how much we can gain from the "lazy"
boucning in page fault.
>>>> The buffer mapped via dma_ops from virtio driver is expected to
be
>>>> accessed by the userspace soon.  It looks to me we can do all
those
>>>> stuffs during dma_map() then things would be greatly
simplified.
>>>>
>>> If so, we need to allocate lots of pages from the pool reserved for
>>> atomic memory allocation requests.
>> This should be fine, a lot of drivers tries to allocate pages in atomic
>> context. The point is to simplify the codes to make it easy to
>> determince the correctness so we can add optimization on top simply by
>> benchmarking the difference.
>>
> OK. I will use this way in the next version.
>
>> E.g we have serveral places that accesses orig_phys:
>>
>> 1) map_page(), write
>> 2) unmap_page(), write
>> 3) page fault handler, read
>>
>> It's not clear to me how they were synchronized. Or if it was
>> synchronzied implicitly (via iova allocator?), we'd better document
it.
> Yes.
>
>> Or simply use spinlock (which is the preferrable way I'd like to
go). We
>> probably don't need to worry too much about the cost of spinlock
since
>> iova allocater use it heavily.
>>
> Actually iova allocator implements a per-CPU cache to optimize it.
>
> Thanks,
> Yongji

Right, but have a quick glance, I guess what you meant is that usually 
there's no lock contention unless cpu hot-plug. This can work but the 
problem is that such synchornization depends on the internal 
implementation of IOVA allocator which is kind of fragile. I still think 
we should do that on our own.

Thanks


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210326/4f3eabe1/attachment.html>