Re: [Qemu-devel] [PATCH 1/9] dma-helpers: Expose the sg mapping logic

[Alex Pyrgiotis <apyrgio@arrikto.com>]

Hi Paolo,

On 02/25/2016 12:22 PM, Paolo Bonzini wrote:
> 
> 
> On 25/02/2016 11:10, Alex Pyrgiotis wrote:
>> 8<... snip ...>8
>>
>> Sure, you're right, there's no sensible way to break an ioctl()
>> operation in many. However, I'd argue that we shouldn't need to, as it
>> would be much better if the DMA operation was never restarted in the
>> first place. Instead, if we dealt with the bigger issue of the global
>> bounce buffer, we could kill two birds with one stone.
>>
>> I see that there was an attempt [1] to replace the global bounce buffer
>> with something more dynamic. In short, the objections [2] were the
>> following:
>>
>> 1. It introduced locking/unlocking a global mutex in the hot path as
>>    well as a hash table lookup.
>> 2. It allowed for unbounded memory allocations.
>>
>> An improvement that would address (1) is to get rid of any global state:
>>
>> Since the mapping operation takes place in the context of a DMA
>> operation, we could provide a ctx-type struct to the dma_memory_(un)map
>> --> address_space_(un)map functions that would contain a hash table. If
>> any memory allocations were needed, we would track them using that hash
>> table, which would require no locks. Moreover, if the initialization of
>> the hash table hurts the performance in the general case, we could use
>> instead a skip list, if the number of memory allocations is small (e.g.
>> < 100).
> 
> You don't need a hash table either if you manage the bounce buffer list
> per DMA transfer, and the simplest way to achieve that is to move the
> bounce buffer from exec.c to dma-helpers.c entirely.
> 
> The patch could first introduce address_space_map_direct that never uses
> the bounce buffer.  dma-helpers.c can call address_space_map_direct and,
> if it fails, proceed to allocate (and fill if writing to the device) a
> bounce buffer.

You mean that address_space_map_direct() is a copy of the
address_space_map() code, minus the bounce buffer part which will be
handled in dma-helpers.c? If so, I agree.

Also, the buffer should be removed from address_space_unmap.

> Since the QEMUSGList is mapped and unmapped
> beginning-to-end, you can just use a FIFO queue.  The FIFO queue stores
> a (QEMUSGList, buffer) tuple.

I suppose that this queue is stored in the dbs structure of dma-helpers?
If so, I agree.

> When unmapping a QEMUSGList you check if
> it matches the head of the queue; if it does, you write back the
> contents of the bounce buffer (for reads from the device) and free it.
> If it doesn't match, you call address_space_unmap.

Agree.

> Then, once the bounce buffer is implemented within dma-helpers.c, you
> remove address_space_map and rename address_space_map_direct to
> address_space_map.  cpu_register_map_client goes away.

What about the other users of address_space_map()? Is the bounce buffer
used only for DMA operations? If so, I agree. Else, we need a fallback.

> The unbounded memory allocation can be avoided by bounding the number of
> entries in the queue.

Agree.

Thanks,
Alex