On 17.03.2016 23:33, David Gibson wrote:
> On Thu, Mar 17, 2016 at 04:15:38PM +0100, Thomas Huth wrote:
>> 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.
>
> Ah, good point. I think I was assuming that the pools got sorted when
> one was emptied as well, but they're not and I suspect it's not a good
> idea to do so.
>
> Hmm.. I wonder if there's a brief way of explaining the above to put
> in the comment.
Something like:
/*
* If the guest used all pools, but changed the size of one pool
* inbetween, we might need to recycle that pool here (if it has
* already been emptied). Thus we need to scan all buffer pools
* here, not only the last one (which has the highest probability
* of being empty)
*/
?
Or is that too verbose already?
Thomas
signature.asc
Description: OpenPGP digital signature
