[ovs-dev] [PATCH v2] datapath: Per cpu flow stats.

[Jesse Gross]

On Fri, Oct 18, 2013 at 1:06 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index faa4e15..d2ca7fc 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> +void ovs_flow_stats_get(struct sw_flow *flow, struct sw_flow_stats *res)
> +{
> +       int cpu;
> +
> +       memset(res, 0, sizeof(*res));
> +
> +       for_each_possible_cpu(cpu) {
> +               struct sw_flow_stats *stats = &flow->stats[cpu];
> +
> +               if (cpu == smp_processor_id())
> +                       local_bh_disable();

I don't think this is safe on preemtable kernels. Couldn't the CPU
switch between smp_processor_id() and local_bh_disable()?

> diff --git a/datapath/flow.h b/datapath/flow.h
> index 91a3022..987e9e5 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> +struct sw_flow_stats {
> +       u64 packet_count;               /* Number of packets matched. */
> +       u64 byte_count;                 /* Number of bytes matched. */
> +       unsigned long used;             /* Last used time (in jiffies). */
> +       spinlock_t lock;                /* Lock for values below. */

The comment on lock could be updated (the values are below anymore).

> +       u8 tcp_flags;                   /* Union of seen TCP flags. */
> +} ____cacheline_aligned;

It probably doesn't matter much but I think we could make this
____cacheline_aligned_in_smp.

> -void ovs_flow_used(struct sw_flow *, struct sk_buff *);
> +void ovs_flow_stats_update(struct sw_flow *flow, struct sk_buff *skb);
> +void ovs_flow_stats_get(struct sw_flow *flow, struct sw_flow_stats *res);
> +void ovs_sw_flow_stats_clear(struct sw_flow *flow);

The "sw_" in the last function name seems a little inconsistent to me.