Re: [Qemu-devel] [PATCH 2/3] hw/net/spapr_llan: Fix receive buffer handling for better performance

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

On 17.03.2016 07:23, David Gibson wrote:
> On Wed, Mar 16, 2016 at 01:16:50PM +0100, Thomas Huth wrote:
>>
>> This patch introduces an alternate way of handling the receive
>> buffers of the spapr-vlan device, resulting in much better
>> receive performance for the guest.
[...]
>> Though it seems at a first glance like PAPR says that we should store
>> the receive buffer descriptors in the page that is supplied during
>> the H_REGISTER_LOGICAL_LAN call, chapter 16.4.1.2 in the LoPAPR spec
>> declares that "the contents of these descriptors are architecturally
>> opaque, none of these descriptors are manipulated by code above
>> the architected interfaces".
> 
> Aaaahhh!  I remember back when I first implemented this, that exposing
> the pool of buffer descriptors via DMA seemed a silly and pointless
> thing to do, but I did so because I thought that's what the spec said.
> 
> 16.4.1.2 seems to make it clearer that the page doesn't list actual Rx
> buffers, but rather opaque handles on internal buffer pools.
> 
> I don't know if I just misread this back in 2011 (or whenever it was)
> or if the PAPR wording at the time was less clear at the time.
> 
> I note that you don't actually put the buffer pool pointers into that
> page in your patch below.  I don't think that matters now, but I
> wonder if we'd ever want to implement H_MIGRATE_DMA and if we'd need
> it in that case.

I also thought about putting the pointers to the pools into that page.
But: If we put buffer pool pointers into that page, where should the
buffer pools be located? Still in the memory of the hypervisor? Then
this sounds like a very baaad design, the guest then could tinker with
pointers to the host memory, causing very bad side effects or crashes.
Or should the buffer pools be located in guest memory? That might be OK,
but how do the pools get allocated in that case?

So unless you've got a good idea here, I think it's better to keep the
pointer list and the buffer pools both in host memory for now.

[...]
>> +/**
>> + * Enqueuing receive buffer by adding it to one of our receive buffer pools
>> + */
>> +static target_long spapr_vlan_add_rxbuf_to_pool(VIOsPAPRVLANDevice *dev,
>> +                                                target_ulong buf)
>> +{
>> +    int size = VLAN_BD_LEN(buf);
>> +    int pool;
>> +
>> +    pool = spapr_vlan_get_rx_pool_id(dev, size);
>> +
>> +    /* No matching pool found? Try to create a new one */
>> +    if (pool < 0) {
>> +        for (pool = RX_MAX_POOLS - 1; pool >= 0 ; pool--) {
> 
> I don't think this loop actually accomplishes anything.  Either the
> last slot is free, in which case you use it, then sort into place, or
> it's not, in which case you've hit the maximum number of buffer pools.

Oh, you're right. Well spotted! I'll rework my patch to do it without
that loop.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]