[ovs-dev] [PATCH v2 02/13] datapath: Per NUMA node flow stats.

Jarno Rajahalme jrajahalme at nicira.com
Thu Feb 13 18:51:03 UTC 2014


On Feb 12, 2014, at 4:12 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> On Tue, Feb 11, 2014 at 4:07 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
...
>> +#endif
> 
> How much overhead do we have for non-NUMA systems without this compile
> time check.
> Since we always allocate node zero stat I do not think update stats
> has that much overhead for that case.
> 

You are right, I removed this.

...
>> 
>> +       spin_lock_bh(&stats->lock);
>> +       *used = stats->used;
>> +       *tcp_flags = stats->tcp_flags;
>> +       ovs_stats->n_packets = stats->packet_count;
>> +       ovs_stats->n_bytes = stats->byte_count;
>> +       spin_unlock_bh(&stats->lock);
>> +
> Is there reason for reading node zero stat outside of for_each_node() {} loop?
> 

Not really.

>> +#if MAX_NUMNODES > 1
>> +       /* Collect stats from other nodes. */
>> +       for_each_node(node) {
>> +               if (node == 0)
>> +                       continue; /* Done already. */
>> +               stats = flow->stats[node];
> 
> You need smp_read_barrier_depends() barrier here.
> As Jesse suggested we can use higher level of API, like rcu_dereference/assign.
> 

Changed to use rcu macros.

...
>> 
>> static int check_header(struct sk_buff *skb, int len)
>> diff --git a/datapath/flow.h b/datapath/flow.h
>> index c4de0e6..f6cce35 100644
>> --- a/datapath/flow.h
>> +++ b/datapath/flow.h
>> @@ -155,6 +155,9 @@ struct flow_stats {
>>        unsigned long used;             /* Last used time (in jiffies). */
>>        spinlock_t lock;                /* Lock for atomic stats update. */
>>        __be16 tcp_flags;               /* Union of seen TCP flags. */
>> +       int last_writer;                /* NUMA-node id of the last writer or
>> +                                        * -1. Meaningful for 'stats[0]' only.
>> +                                        */
>> };
>> 
> I did not understood your comment from last thread about moving it to
> struct sw_flow.
> Since flow->stat[0] is only shared why we need last_writer in each flow_stats?
> Therefore we should be able to move it to struct sw_flow.

Right. However, the code in ovs_flow_stats_update() becomes a bit more complicated. Let me know what you think.

To keep things simple I’ll post a new version of this patch separately from the rest of the series.

  Jarno

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140213/d3011175/attachment-0003.html>


More information about the dev mailing list