On 24.02.2016 16:20, Flavio Leitner wrote:
> On Wed, 24 Feb 2016 10:54:20 +0300
> Ilya Maximets <i.maxim...@samsung.com> wrote:
>
>> On 22.02.2016 19:10, Flavio Leitner wrote:
>>>> Reviewed-by: Aaron Conole <acon...@redhat.com>
>>>> Acked-by: Flavio Leitner <f...@sysclose.org>
>>>
>>> If you do small changes to the patch that doesn't alter its logic you
>>> can preserve the signature from others (e.g.: typos, indentation,
>>> comments...). However, in this case you changed the logic so you can't
>>> preserve those anymore.
>>>
>>> The procedure would be to take them out and add the previous reviewers
>>> to the CC in the hope that they will review the new patch again.
>>
>> Flavio, Aaron,
>> Really sorry for that copy-pasted header. I'll try to be more careful.
>>
>>>> @@ -623,6 +628,8 @@ netdev_dpdk_init(struct netdev *netdev_, unsigned int
>>>> port_no,
>>>> if (err) {
>>>> goto unlock;
>>>> }
>>>> + } else {
>>>> + netdev_dpdk_alloc_txq(netdev, VHOST_MAX_QUEUE_PAIRS);
>>>
>>> The VHOST_MAX_QUEUE_PAIRS is 0x8000, so we are allocating 32768 queue.
>>> Also that the struct dpdk_tx_queue has 3096 bytes so in the end it is
>>> allocating 101MB for each vhost-user port.
>>>
>>> Why do we need to pre-allocate all TX queues?
>>
>> Main reasons:
>> * First signal from vhost_thread may be received between netdev_open()
>> and netdev_set_multiq(). This will lead to segfault in previous
>> version of this patch due to unallocated tx_q[].
>> * We don't know real number of TX queues before new_device() call, but
>> vring_state_changed() usually called before new_device().
>>
>> There are two ways to avoid above problems:
>> 1. Preallocate all TX queues.
>> I agree that VHOST_MAX_QUEUE_PAIRS is too much. We can use own
>> constant here:
>> #define OVS_VHOST_MAX_QUEUE_NUM 1024
>> 1024 will help to reduce memory consumption to ~3 Mb per port.
>> This constant is from QEMU:
>> include/net/net.h:#define MAX_QUEUE_NUM 1024
>> This must be sane to limit number of queues by number supported
>> by QEMU.
>>
>> 2. Reallocation of tx_q[] each time inside vring_state_changed() and
>> new_device() with preserving of the previous content and initializing
>> of newly allocated memory.
>>
>> First solution was chosen just because it's much simpler to implement.
>> I'd like to replace VHOST_MAX_QUEUE_PAIRS with OVS_VHOST_MAX_QUEUE_NUM.
>> What to you think about all this?
>
> OK, I think we can have the new macro using less queues for now and
> then optimize the code to allocate on demand later.
>
> Could you please spin a new version?
Just sent.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
