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

Pravin Shelar pshelar at nicira.com
Fri Oct 18 20:33:24 UTC 2013


On Fri, Oct 18, 2013 at 1:21 PM, Jesse Gross <jesse at nicira.com> wrote:
> 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()?
>
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.



More information about the dev mailing list