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