Hi Mark,

Thank you so much for sharing this data. Please see my comments inline.

On Fri, May 18, 2018 at 1:31 PM, Mark Michelson <mmich...@redhat.com> wrote:

> Hi Han, I finally did some tests and looked at the CPU usage between
> master and the ip7 branch.
>
> On the machines running ovn-controller:
> Master branch: Climbs to around 100% over the course of 3 minutes, then
> oscillates close to 100% for about 10 minutes, and then is pegged to 100%
> for the rest of the test. Total test time was about 23 minutes.
> ip7 branch: oscillates between 10 and 25% for the first 10 minutes of the
> test, then hovers around 10% for the rest. Total test time was about 19
> minutes.
>
> This is aligned with my observation of ~90% improvement on CPU cost.

For the throughput/total time, the improvement ratio is different (in my
test case the execution time reduced ~50%) but I think it can be explained.
The total execution time is not accurately reflecting the efficiency of the
processing, because when CPU is 100%, ovn-controller processing will be
slowed down which may just end up less iterations during the whole test. I
think the stop-watch profiling mechanism you implemented (also rebased into
the incremental processing) will be able to tell the truth. The real impact
of that is longer latency for handling a change in control plane. So I also
use latency to evaluate the improvement. The way I test latency is using
ovn-nbctl --wait=hv, with the nb_cfg improvement (
https://patchwork.ozlabs.org/patch/899608/).

When I switched over to tests that have ACLs:
> Master branch: Behaves about the same as the master branch when no ACLs
> are used. Total test time was about 28 minutes
> ip7 branch: CPU usage hovered around 30% for the entirety of the test,
> hitting spikes around 50% a couple of times. Total test time was about 25
> minutes.
>
> Since I had not done it yet, I also ran perf while running the incremental
> branch with ACLs. I am attaching the flame graph here. The gist is that
> much like the master branch, the majority of CPU time is spent processing
> logical flows.
>
> Seeing the drop in CPU usage between the master branch and the ip7 branch
> makes me think it is worth investigating other areas that may be the
> bottleneck. I monitored memory, disk usage, and network usage on the
> machines, but I didn't see anything that seemed obvious as being the cause
> for delay.
>
> The CPU drop between master and ip7 when testing with ACLs, for my
understanding, most likely because of incremental processing avoids
recompute flows when irrelevant input such as pinctrl/ofctrl messages (e.g.
probe/echo) comes, while in master any of these inputs would trigger
recomputing.


> CPU-wise, I think the biggest improvements that can be made to the
> incremental processing branch are:
> * Adding a change handler for the Address_Set table.
> * ofctrl_put() improvements we have discussed.
>
> I think this will have noticeable improvements in our test times. However,
> based on how much the CPU usage dropped just from switching to the
> incremental processing branch, I think there are likely some other
> bottlenecks in our tests that would be more impactful to remove. We already
> know that "ovn_network.bind_port" and "ovn_network.wait_port_up" in
> ovn-scale-test terminology are the operations in our test iterations that
> take the longest. If we can break those down into smaller pieces, we can
> potentially zero in on what to target next.
>

I am not sure if there is any other *big* bottlenecks, but
address-set/port-group and ofctrl_put() improvement are surely needed :)
The latest patch I provided is from my ip9 branch, which is rebased on
master this week, with some code refactors. Feel free to try it, but don't
expect any performance difference.

Thanks,
Han

>
> On 05/11/2018 12:50 PM, Han Zhou wrote:
>
>>
>>
>> On Fri, May 11, 2018 at 7:58 AM, Mark Michelson <mmich...@redhat.com
>> <mailto:mmich...@redhat.com>> wrote:
>>
>>     On 05/10/2018 08:33 PM, Han Zhou wrote:
>>
>>
>>
>>         On Wed, May 9, 2018 at 12:21 PM, Mark Michelson
>>         <mmich...@redhat.com <mailto:mmich...@redhat.com>
>>         <mailto:mmich...@redhat.com <mailto:mmich...@redhat.com>>> wrote:
>>
>>              On 05/07/2018 08:29 AM, Mark Michelson wrote:
>>
>>                  On 05/04/2018 04:15 PM, Han Zhou wrote:
>>
>>
>>
>>                      On Thu, May 3, 2018 at 10:49 AM, Mark Michelson
>>                      <mmich...@redhat.com <mailto:mmich...@redhat.com>
>>         <mailto:mmich...@redhat.com <mailto:mmich...@redhat.com>>
>>                      <mailto:mmich...@redhat.com
>>         <mailto:mmich...@redhat.com> <mailto:mmich...@redhat.com
>>         <mailto:mmich...@redhat.com>>>>
>>
>>                      wrote:
>>
>>                           On 05/03/2018 12:58 PM, Mark Michelson wrote:
>>
>>                               Hi Han,
>>
>>                               (cc'ed ovs-discuss)
>>
>>                               I have some test results here for your
>>         incremental
>>                      branch[0].
>>
>>                               Browbeat was the test orchestrator for the
>>         test,
>>                      and it uses
>>                               ovn-scale-test[1] to configure the test
>>         parameters
>>                      and run the test.
>>
>>                               The test use one central node on which
>>         ovn-northd
>>                      runs. There
>>                               are three farm nodes on which sandboxes
>>         are run for
>>                               ovn-controller. Each farm node runs 24
>>         sandboxes,
>>                      for a total of
>>                               72 sandboxes (and thus 72 ovn-controller
>>         processes).
>>
>>                               The test uses the datapath context[2] to
>>         set up 72
>>                      logical
>>                               switches and one logical router in
>>         advance. Then
>>                      during each
>>                               test iteration, a logical switch port is
>>         added to
>>                      one of the
>>                               logical switches and bound on one of the
>>         sandboxes.
>>                      The next
>>                               iteration does not start until the previous
>>                      iteration is 100%
>>                               complete (i.e. we see that the logical
>>         switch port
>>                      is "up" in
>>                               the northbound db). The total number of
>>         logical
>>                      switch ports
>>                               added during the tests is 3312.
>>
>>                               During the test, I ran `perf record` on
>>         one of the
>>                               ovn-controller processes and then created
>>         a flame
>>                      graph[3] from
>>                               the results. I have attached the flame
>>         graph to
>>                      this e-mail. I
>>                               think this can give us a good jumping off
>>         point for
>>                      determining
>>                               more optimizations to make to
>> ovn-controller.
>>
>>                               [0] https://github.com/hzhou8/ovs/tree/ip7
>>         <https://github.com/hzhou8/ovs/tree/ip7>
>>                      <https://github.com/hzhou8/ovs/tree/ip7
>>         <https://github.com/hzhou8/ovs/tree/ip7>>
>>                               <https://github.com/hzhou8/ovs/tree/ip7
>>         <https://github.com/hzhou8/ovs/tree/ip7>
>>                      <https://github.com/hzhou8/ovs/tree/ip7
>>         <https://github.com/hzhou8/ovs/tree/ip7>>>
>>                               [1]
>>         https://github.com/openvswitch/ovn-scale-test
>>         <https://github.com/openvswitch/ovn-scale-test>
>>                      <https://github.com/openvswitch/ovn-scale-test
>>         <https://github.com/openvswitch/ovn-scale-test>>
>>                                      <https://github.com/openvswit
>> ch/ovn-scale-test
>>         <https://github.com/openvswitch/ovn-scale-test>
>>                      <https://github.com/openvswitch/ovn-scale-test
>>         <https://github.com/openvswitch/ovn-scale-test>>>
>>                               [2]
>>         https://github.com/openvswitch/ovn-scale-test/pull/165
>>         <https://github.com/openvswitch/ovn-scale-test/pull/165>
>>                             <https://github.com/openvswitc
>> h/ovn-scale-test/pull/165
>>         <https://github.com/openvswitch/ovn-scale-test/pull/165>>
>>                                                 <
>> https://github.com/openvswitch/ovn-scale-test/pull/165
>>         <https://github.com/openvswitch/ovn-scale-test/pull/165>
>>                             <https://github.com/openvswitc
>> h/ovn-scale-test/pull/165
>>         <https://github.com/openvswitch/ovn-scale-test/pull/165>>>
>>                               [3]
>>         http://www.brendangregg.com/FlameGraphs/cpuflamegraphs
>>         <http://www.brendangregg.com/FlameGraphs/cpuflamegraphs>
>>                             <http://www.brendangregg.com/F
>> lameGraphs/cpuflamegraphs
>>         <http://www.brendangregg.com/FlameGraphs/cpuflamegraphs>>
>>                                                 <
>> http://www.brendangregg.com/FlameGraphs/cpuflamegraphs
>>         <http://www.brendangregg.com/FlameGraphs/cpuflamegraphs>
>>                             <http://www.brendangregg.com/F
>> lameGraphs/cpuflamegraphs
>>         <http://www.brendangregg.com/FlameGraphs/cpuflamegraphs>>>
>>
>>
>>
>>                            From the IRC meeting, it was requested to see
>>         a flame
>>                      graph of
>>                           performance on OVS master. I am attaching that
>>         on this
>>                      e-mail.
>>
>>                           One difference in this test run is that the
>>         number of
>>                      switch ports
>>                           was fewer (I'm not 100% sure of the exact
>>         number), so
>>                      the number of
>>                           samples in perf record is less than in the
>>         flame graph
>>                      I previously
>>                           sent.
>>
>>                           The vast majority of the time is spent in
>>         lflow_run().
>>                      Based on this
>>                           flame graph, our initial take on the matter
>>         was that we
>>                      could get
>>                           improved performance by reducing the number of
>>         logical
>>                      flows to
>>                           process. The incremental branch seemed like a
>> good
>>                      testing target to
>>                           that end.
>>
>>
>>                      Thanks Mark for sharing the results!
>>                      It seems you have sent the wrong attachment
>>         perf-master.svg
>>                      in your second email, which is still the same one
>>         as in the
>>                      first email. Would you mind sending the right one?
>>         Also,
>>                      please if you could share total CPU cost for
>>         incremental
>>                      v.s. master, when you have the data.
>>
>>                        From your text description, it is improved as
>>         expected,
>>                      since the bottleneck moved from lflow_run() to
>>         ofctrl_put().
>>                      For the new bottleneck ofctrl_put(), it's a good
>>         finding,
>>                      and I think I have some ideas to improve that, too.
>>                      Basically, when we are able to do incremental
>>         computing, we
>>                      don't need to go through and compare the installed
>> v.s.
>>                      desired flows every time, but instead we can
>>         maintain a list
>>                      of changes made in the iteration and then send them
>>         to OVS.
>>                      This would reduce a lot of ovn_flow_lookup() calls
>>         which is
>>                      currently shown as the hotspot in ofctrl_put(). I
>>         will try
>>                      this as soon as I fix some corner cases and get
>>         confident
>>                      about the correctness.
>>
>>                      Thanks,
>>                      Han
>>
>>
>>                  Apologies, Han. Here is the proper file.
>>
>>                  Your conclusion is similar to what I thought about when
>>         I saw
>>                  where the new bottleneck was.
>>
>>                  I don't have a comparison of total CPU cost yet. But I
>>         can tell
>>                  you that from observing CPU usage in real time during
>>         the test
>>                  with the master branch, the graphs appeared to be at
>>         100% across
>>                  all CPU cores for the duration of the test. I have not
>>         observed
>>                  CPU usage during a test with the incremental branch to
>>         see if it
>>                  improved.
>>
>>                  Mark!
>>
>>
>>              Hello Han,
>>
>>              Just following up on the previous e-mail. I plan to run a
>>         test soon
>>              with the incremental branch and observe CPU usage. I won't
>>         be able
>>              to share a graph, but I can at least tell you if I observe an
>>              overall drop in CPU usage compared to using the master
>> branch.
>>
>>              I took a look through ofctrl_put() and I think that the
>>         incremental
>>              processing branch puts it in a good position to be improved
>>              dramatically.
>>
>>              In master, we currently maintain a hmap of installed flows.
>>         When a
>>              loop of ovn-controller runs, we use the contents of the
>>         southbound
>>              database to build up a hmap of our desired flows. We then
>>         perform an
>>              (expensive) operation where we reconcile the installed
>>         flows with
>>              the desired flows. The output of all of this is a list of
>>              ofputil_flow_mods that we then pass down to vswitchd.
>>
>>              In the incremental processing branch [1], this is helped a
>>         bit. When
>>              we can perform incremental processing, the desired flows are
>> no
>>              longer built from scratch on each loop. Instead, the
>>         desired flows
>>              persist and are modified based on changes observed in the
>>         database.
>>              ofctrl_put() is mostly unchanged, although it can exit
>>         early if it
>>              knows that there were no changes observed.
>>
>>         In fact the ofctrl_put() (in ip7 branch) already takes care of
>>         the no change scenario. It returns immediately if decided no
>>         change needed after some simple evaluation ("flow_changed" is
>>         one of the condition it considers). But yes, when change is
>>         needed, it can be further optimized as I mentioned also in last
>>         email.
>>
>>
>>              I've thought about this and considered two stages to
>>         potentially
>>              improving this.
>>
>>              Stage 1: When performing incremental processing, don't bother
>>              dealing with desired flows at all. Instead, when changes are
>>              observed in the southbound database, modify the installed
>> flows
>>              directly and create ofputil_flow_mods based on the changes.
>>         When it
>>              is time to run ofctrl_put(), we can pass the generated
>>              ofputil_flow_mods directly to vswitchd without having to
>>         perform any
>>              sort of reconciliation between hmaps. We still have to
>>         maintain the
>>              installed flows because full runs through the southbound
>>         database
>>              will need to perform the "classic" version of ofctrl_put()
>>         and will
>>              need to have an up-to-date table of installed flows.
>>
>>         I was thinking about the same, but then figured out we still
>>         need the desired flows because of a corner case: different
>>         logical flows can generate same OVS flows (e.g. when 2 identical
>>         ACLs are created for the same ovn port, due to client behavior,
>>         which is real at least in Neutron), but duplicated flows can't
>>         exist in installed flows. If we don't maintain desired flows
>>         that stores multiple entries of duplicated flows, when we handle
>>         a logical flow deletion we don't know if the corresponding
>>         installed flow(s) is/are still being used by any other logical
>> flow.
>>
>>
>>     I'm glad I asked because I would not have realized that distinct
>>     logical flows could result in the same OVS flow.
>>
>>     One idea about how to address this issue is that the installed_flows
>>     hmap could consist of both the OVS flow and a reference count. The
>>     reference count is equal to the number of logical flows that
>>     resulted in that OVS flow. When doing incremental processing, it's
>>     easy to add and remove from the count based on changes in logical
>>     flows. If the count on an installed OVS flow reaches 0, then the
>>     flow can be removed from the hmap.
>>
>>
>>         This can be addressed by changing the installed flow table
>>         structure to maintain the M:N mapping between logical flow and
>>         OVS flows, and then the desired flow table may be removed.
>>         However, keeping the desired flow table doesn't harm either
>>         (despite the memory consumption), and I am not sure if there are
>>         other reasons we will need it (maybe it helps when full
>>         computation is needed). When incremental processing taking
>>         effect, we can still avoid the costly full table comparing by
>>         maintaining a list of the changes while keep updating desired
>>         flow table.
>>
>>
>>     My thinking behind removing the persistent desired flows table was
>>     mainly just removing something that didn't need to be there. For
>>     full computation, we can do what OVS master currently does: build
>>     the desired flows from scratch and then delete them at the end of
>>     the iteration. Thinking about it more, I think you're right that the
>>     main savings we would get would be memory consumption.
>>
>>
>>              Stage 2: If the code ever reaches a point where incremental
>>              processing is guaranteed to be performed on every loop
>>         iteration of
>>              ovn-controller, then ovn-controller could become stateless
>> with
>>              regards to installed flows. There would be no need to
>>         maintain the
>>              installed flows since we could just generate
>>         ofputil_flow_mods based
>>              on how the southbound database has changed. We could then
>>         pass those
>>              modifications down to vswitchd.
>>
>>         This would be ideal situation, but it seems to me not very
>>         realistic for ovn-controller, since there are just too many
>>         dependencies. Probably I am just not convinced of the value v.s.
>>         effort to achieve that. Maybe it is ok to recompute for changes
>>         that not happen very often, isn't it?
>>
>>
>>     I agree that it is not very realistic, but it would be nice if it
>>     were possible to do. I agree that for infrequent changes, performing
>>     a recompute is probably fine.
>>
>>
>>              I think similar changes could be implemented for the group
>>         and meter
>>              extend tables, but they are not nearly as important as the
>>         flow table.
>>
>>              Do my suggestions make sense to you? Are they in line with
>>         what you
>>              had been thinking about?
>>
>>         Of course! Thanks for sharing your good thoughts. I am glad this
>>         work helps and let's improve it further :)
>>
>>              Thanks,
>>              Mark
>>
>>              [1] Currently I'm looking at
>>         https://github.com/hzhou8/tree/ip7
>>         <https://github.com/hzhou8/tree/ip7>
>>              <https://github.com/hzhou8/tree/ip7
>>         <https://github.com/hzhou8/tree/ip7>> . I noticed that there is
>> an
>>              ip8 branch as well, but since I have been using ip7 in
>> tests, I
>>              decided look at it as a reference.
>>
>>
>>         I will keep the branch ip7 untouched and let you know when I get
>>         ip8 ready.
>>
>>
>>     Thanks. Can you give a brief summary of what is different in ip8?
>>     Just a sentence or two is fine.
>>
>>
>> All new updates will go to ip8 branch, until a new one is needed. It will
>> at least include rebase again on master, and some corner case handling.
>> Probably it will also include address set and port-group incremental
>> processing later.
>>
>>
>>
>
_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Reply via email to