> On Jun 23, 2016, at 2:51 AM, Jan Scheurich <jan.scheur...@ericsson.com> wrote: > >> This no longer applies cleanly, and I also needed to add this incremental to >> make the testsuite pass: > > I will rebase and adapt the tests as suggested in the next version. > >> Did you consider keeping a separate dpcls instance per input port instead? >> I.e., have an hmap of dpcls'es using the in_port as the hash key, and >> creating >> and destroying them on demand. This would avoid the fixed size array, >> simplify the code and each insert and remove operation would be a bit >> cheaper. A miss would typically go though a lower number of subtables, even >> though the upcall cost would likely hide this benefit in practice. We are >> always exact matching in_port (as well as dl_type and recirc_id (see >> xlate_wc_init()), so this would add no overhead in terms of the overall >> number of megaflows. >> > > I see your point. I guess this would be a somewhat bigger change, but I can > have a look at it. >
While the change is a bit bigger conceptually, in terms of code changes it might not be that bad. > I am a bit worried about the memory footprint and the impact on CPU cache > performance because each PMD core would have its own set of dpcls'es even, > although with RSS and vhost multi-queue support, each in_port will typically > be served by many PMD cores. I guess only a proper performance test exercising this will tell if it makes a difference. > > In theory it should be enough to have a single dpcls instance per in_port > shared by all PMDs which contains all the read-only data. Each PMD would > cache relevant parts of that in its private L1 cache, so there should not be > any performance impact. Volatile data such as flow stats would have to be > kept per PMD thread as today. This should also avoid multiple up-calls > installing the same megaflow in different PMD dpcls'es. > > What do you think? Write access to cmaps requires mutual exclusion lock among writers which might hurt flow set-up performance. Also, if multiple packets of a single flow are processed by multiple PMD threads, they would likely all execute the upcalls anyway, and then lock each other out while trying to install the flow in the dpcls. We actually started with a single dpcls for the entire userspace datapath and the current division to one per PMD thread was introduced as an optimization to avoid this locking penalty. > >> Also, it should be possible to expand the pvector API to allow using the >> pvector priorities directly as the counts. I posted a patch for this >> (https://patchwork.ozlabs.org/patch/638920/), after which you could apply >> the following incremental. I haven't tested how this performs, though. > > I had a look at your patches. I will give that a try to check the performance > gain. > > One question, though. Wouldn't it be cleaner to create a new pvector data > type variant without the multithread safety. I have the impression that the > client code in dpif-netdev.c is now more exposed to implementation details of > pvector than it should be, or? Essentially pvector_impl is now the variant without thread safety. I contemplated naming the pvector_impl to something else as it is now exposed, but did not come up with a good name. Suggestions? Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
