netdev - Re: [PATCH 2/2] openvswitch: Optimize operations for OvS flow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOrHB_AVZP3DVER5wrU_hBL-8uHcMBKPL+xNDv_mOZxKRVPopw@mail.gmail.com>
Date:   Mon, 17 Jul 2017 20:14:35 -0700
From:   Pravin Shelar <pshelar@....org>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH 2/2] openvswitch: Optimize operations for OvS flow_stats.

On Sun, Jul 16, 2017 at 11:29 PM, Tonghao Zhang
<xiangxia.m.yue@...il.com> wrote:
> When calling the flow_free() to free the flow, we call many times
> (cpu_possible_mask, eg. 128 as default) cpumask_next(). That will
> take up our CPU usage if we call the flow_free() frequently.
> When we put all packets to userspace via upcall, and OvS will send
> them back via netlink to ovs_packet_cmd_execute(will call flow_free).
>
> The test topo is shown as below. VM01 sends TCP packets to VM02,
> and OvS forward packtets. When testing, we use perf to report the
> system performance.
>
> VM01 --- OvS-VM --- VM02
>
> Without this patch, perf-top show as below: The flow_free() is
> 3.02% CPU usage.
>
>         4.23%  [kernel]            [k] _raw_spin_unlock_irqrestore
>         3.62%  [kernel]            [k] __do_softirq
>         3.16%  [kernel]            [k] __memcpy
>         3.02%  [kernel]            [k] flow_free
>         2.42%  libc-2.17.so        [.] __memcpy_ssse3_back
>         2.18%  [kernel]            [k] copy_user_generic_unrolled
>         2.17%  [kernel]            [k] find_next_bit
>
> When applied this patch, perf-top show as below: Not shown on
> the list anymore.
>
>         4.11%  [kernel]            [k] _raw_spin_unlock_irqrestore
>         3.79%  [kernel]            [k] __do_softirq
>         3.46%  [kernel]            [k] __memcpy
>         2.73%  libc-2.17.so        [.] __memcpy_ssse3_back
>         2.25%  [kernel]            [k] copy_user_generic_unrolled
>         1.89%  libc-2.17.so        [.] _int_malloc
>         1.53%  ovs-vswitchd        [.] xlate_actions
>
> With this patch, the TCP throughput(we dont use Megaflow Cache
> + Microflow Cache) between VMs is 1.18Gbs/sec up to 1.30Gbs/sec
> (maybe ~10% performance imporve).
>
> This patch adds cpumask struct, the cpu_used_mask stores the cpu_id
> that the flow used. And we only check the flow_stats on the cpu we
> used, and it is unncessary to check all possible cpu when getting,
> cleaning, and updating the flow_stats. Adding the cpu_used_mask to
> sw_flow struct does’t increase the cacheline number.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>

Patch looks good to me. I have one comment.

> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index ea7a807..324a246 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -98,6 +98,9 @@ struct sw_flow *ovs_flow_alloc(void)
>
>         RCU_INIT_POINTER(flow->stats[0], stats);
>
> +       cpumask_clear(&flow->cpu_used_mask);
I do not see need to clear the cpumask when flow is already zeroed.

> +       cpumask_set_cpu(0, &flow->cpu_used_mask);
> +
>         return flow;
>  err:
>         kmem_cache_free(flow_cache, flow);
> @@ -141,7 +144,7 @@ static void flow_free(struct sw_flow *flow)
>         if (flow->sf_acts)
>                 ovs_nla_free_flow_actions((struct sw_flow_actions __force *)flow->sf_acts);
>         /* We open code this to make sure cpu 0 is always considered */
> -       for (cpu = 0; cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpu_possible_mask))
> +       for (cpu = 0; cpu < nr_cpu_ids; cpu = cpumask_next(cpu, &flow->cpu_used_mask))
>                 if (flow->stats[cpu])
>                         kmem_cache_free(flow_stats_cache,
>                                         (struct flow_stats __force *)flow->stats[cpu]);
> --
> 1.8.3.1
>