Re: [PATCH 2/4] swiotlb: Add a new cc-swiotlb implementation for Confidential VMs

[Guorui Yu <GuoRui.Yu@linux.alibaba.com>]

Hi Andi,

在 2023/1/30 14:46, Andi Kleen 写道:
> 
>> I try to solve this problem by creating a new kernel thread, "kccd", 
>> to populate the TLB buffer in the backgroud.
>>
>> Specifically,
>> 1. A new kernel thread is created with the help of "arch_initcall", 
>> and this kthread is responsible for memory allocation and setting 
>> memory attributes (private or shared);
>> 2. The "swiotlb_tbl_map_single" routine only use the spin_lock 
>> protected TLB buffers pre-allocated by the kthread;
>>   a) which actually includes ONE memory allocation brought by xarray 
>> insertion "__xa_insert__".
> 
> That already seems dangerous with all the usual problems of memory 
> allocations in IO paths. Normally code at least uses a mempool to avoid 
> the worst dead lock potential.
> 

The __xa_insert__ is called with GFP_NOWAIT (GFP_ATOMIC & ~__GFP_HIGH), 
and I will try to dig into this to check if there is any chance to have 
the deadlock.

I also tried my best to test this piece of code, and no issue have been 
found in the case of a maximum of 700,000 IOPS.

Thanks for your advices from this point, since I have not notice such 
possibility.

>> 3. After each allocation, the water level of TLB resources will be 
>> checked. If the current TLB resources are found to be lower than the 
>> preset value (half of the watermark), the kthread will be awakened to 
>> fill them.
>> 4. The TLB buffer allocation in the kthread is batched to 
>> "(MAX_ORDER_NR_PAGES << PAGE_SHIFT)" to reduce the holding time of 
>> spin_lock and number of calls to set_memory_decrypted().
> 
> Okay, but does this guarantee that it will never run out of memory?
>
> It seems difficult to make such guarantees. What happens for example if 
> the background thread gets starved by something higher priority?
> 
No, this cannot guarantee we always have sufficient TLB caches, so we 
can also have a "No memory for cc-swiotlb buffer" warning.

But I want to emphasize that in this case, the current implementation is 
no worse than the legacy implementation. Moreover, dynamic TLB 
allocation is more suitable for situations where more disks/network 
devices will be hotplugged, in which case you cannot pre-set a 
reasonable value.

> Or if the allocators have such high bandwidth that they can overwhelm 
> any reasonable background thread.
> 
> -Andi
> 

Sincerely,
Guorui