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: 1943 bytes --]

On 17.03.2016 08:30, Thomas Huth wrote:
> 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.
[...]
>>> +/**
>>> + * 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.

Wait, no, there was a case where this loop is actually really required:

1) All pools are in use and filled with at least one BD
2) User in the guest suddenly decides to change the buffer size of
   one of the pools in the /sys fs of the guest.
3) Guest driver tries to add buffers with a new size that do not
   match any size of one of the pools in the host
4) After the pool on the host runs empty which contained the BDs with
   the size that is not in use anymore, we should recycle that pool
   for the buffers with the new size instead. Since that buffer pool
   might not be at the end of the list, we've got to scan all buffers
   here to make sure we find it.

So I think the for-loop should stay as it is.

 Thomas



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