On Fri, Oct 18, 2013 at 1:06 PM, Pravin B Shelar <pshe...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
