On Fri, Oct 18, 2013 at 1:21 PM, Jesse Gross <je...@nicira.com> wrote:
> 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()?
>
ok, I will disable preemption too.
>> 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).
>
ok.
>> + 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.
>
____cacheline_aligned_in_smp also assigns data section, so I can not
use it for struct definition.
>> -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.
ok, I will remove it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
