[ovs-dev] [PATCH v5 3/7] dpif-netdev: Make datapath and flow stats atomic.

Daniele Di Proietto diproiettod at vmware.com
Tue Apr 7 18:57:20 UTC 2015


> On 3 Apr 2015, at 02:19, Ethan Jackson <ethan at nicira.com> wrote:
> 
> Do we have to use the ATOMIC(long long) syntax, or can we use
> atomic_ullong for most of the struct variable definitions?  If we have
> to use the former, there should be a comment explaining why.
> 

I’ve changed it to atomic_ullong

> In dp_netdev_flow_used() I don't think we need to read the time and do
> a MAX.  time_msec() is monotonic, so just writing the value should be
> fine.
> 

That’s an improvement, thanks

> There should be a comment in dp_netdev_flow_used() explaining why we
> do updates the way we do.
> 
> Alternatively, the patch might benefit from a "relaxed add" helper
> local to dpif_netdev which does a relaxed read, add, relaxed write.
> 

I’ve added the helper to save some lines of code.

> Acked-by: Ethan Jackson <ethan at nicira.com>
> 

Thanks for the review!

> 
> On Wed, Apr 1, 2015 at 9:20 AM, Daniele Di Proietto
> <diproiettod at vmware.com> wrote:
>> A read operation from a non atomic shared value (without external
>> locking) can return incorrect values.  Using the atomic semantics
>> prevents this from happening.
>> 
>> However:
>> * No memory barriers are used.  We don't need that kind of consistency
>>  for statistics (we use relaxed operations).
>> * The updates are not atomic, just the loads and stores.  This is ok
>>  because there's a single writer.
>> 
>> Suggested-by: Ethan Jackson <ethan at nicira.com>
>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>> ---
>> lib/dpif-netdev.c | 69 ++++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 50 insertions(+), 19 deletions(-)
>> 
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index b5cfdcb..a882e9c 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -239,10 +239,10 @@ struct dp_netdev_port {
>> 
>> /* Contained by struct dp_netdev_flow's 'stats' member.  */
>> struct dp_netdev_flow_stats {
>> -    long long int used;             /* Last used time, in monotonic msecs. */
>> -    long long int packet_count;     /* Number of packets matched. */
>> -    long long int byte_count;       /* Number of bytes matched. */
>> -    uint16_t tcp_flags;             /* Bitwise-OR of seen tcp_flags values. */
>> +    ATOMIC(long long) used;         /* Last used time, in monotonic msecs. */
>> +    ATOMIC(long long) packet_count; /* Number of packets matched. */
>> +    ATOMIC(long long) byte_count;   /* Number of bytes matched. */
>> +    ATOMIC(uint16_t) tcp_flags;     /* Bitwise-OR of seen tcp_flags values. */
>> };
>> 
>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'.
>> @@ -338,7 +338,7 @@ static void dp_netdev_actions_free(struct dp_netdev_actions *);
>> /* Contained by struct dp_netdev_pmd_thread's 'stats' member.  */
>> struct dp_netdev_pmd_stats {
>>     /* Indexed by DP_STAT_*. */
>> -    unsigned long long int n[DP_N_STATS];
>> +    ATOMIC(unsigned long long) n[DP_N_STATS];
>> };
>> 
>> /* PMD: Poll modes drivers.  PMD accesses devices via polling to eliminate
>> @@ -754,10 +754,15 @@ dpif_netdev_get_stats(const struct dpif *dpif, struct dpif_dp_stats *stats)
>> 
>>     stats->n_flows = stats->n_hit = stats->n_missed = stats->n_lost = 0;
>>     CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>> +        unsigned long long n;
>>         stats->n_flows += cmap_count(&pmd->flow_table);
>> -        stats->n_hit += pmd->stats.n[DP_STAT_HIT];
>> -        stats->n_missed += pmd->stats.n[DP_STAT_MISS];
>> -        stats->n_lost += pmd->stats.n[DP_STAT_LOST];
>> +
>> +        atomic_read_relaxed(&pmd->stats.n[DP_STAT_HIT], &n);
>> +        stats->n_hit += n;
>> +        atomic_read_relaxed(&pmd->stats.n[DP_STAT_MISS], &n);
>> +        stats->n_missed += n;
>> +        atomic_read_relaxed(&pmd->stats.n[DP_STAT_LOST], &n);
>> +        stats->n_lost += n;
>>     }
>>     stats->n_masks = UINT32_MAX;
>>     stats->n_mask_hit = UINT64_MAX;
>> @@ -1545,13 +1550,23 @@ dp_netdev_pmd_find_flow(const struct dp_netdev_pmd_thread *pmd,
>> }
>> 
>> static void
>> -get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow,
>> +get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_,
>>                     struct dpif_flow_stats *stats)
>> {
>> -    stats->n_packets = netdev_flow->stats.packet_count;
>> -    stats->n_bytes = netdev_flow->stats.byte_count;
>> -    stats->used = netdev_flow->stats.used;
>> -    stats->tcp_flags = netdev_flow->stats.tcp_flags;
>> +    struct dp_netdev_flow *netdev_flow;
>> +    long long n;
>> +    uint16_t flags;
>> +
>> +    netdev_flow = CONST_CAST(struct dp_netdev_flow *, netdev_flow_);
>> +
>> +    atomic_read_relaxed(&netdev_flow->stats.packet_count, &n);
>> +    stats->n_packets = n;
>> +    atomic_read_relaxed(&netdev_flow->stats.byte_count, &n);
>> +    stats->n_bytes = n;
>> +    atomic_read_relaxed(&netdev_flow->stats.used, &n);
>> +    stats->used = n;
>> +    atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags);
>> +    stats->tcp_flags = flags;
>> }
>> 
>> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for
>> @@ -2678,19 +2693,35 @@ static void
>> dp_netdev_flow_used(struct dp_netdev_flow *netdev_flow, int cnt, int size,
>>                     uint16_t tcp_flags)
>> {
>> -    long long int now = time_msec();
>> +    long long n, now = time_msec();
>> +    uint16_t flags;
>> 
>> -    netdev_flow->stats.used = MAX(now, netdev_flow->stats.used);
>> -    netdev_flow->stats.packet_count += cnt;
>> -    netdev_flow->stats.byte_count += size;
>> -    netdev_flow->stats.tcp_flags |= tcp_flags;
>> +    atomic_read_relaxed(&netdev_flow->stats.used, &n);
>> +    n = MAX(now, n);
>> +    atomic_store_relaxed(&netdev_flow->stats.used, n);
>> +
>> +    atomic_read_relaxed(&netdev_flow->stats.packet_count, &n);
>> +    n += cnt;
>> +    atomic_store_relaxed(&netdev_flow->stats.packet_count, n);
>> +
>> +    atomic_read_relaxed(&netdev_flow->stats.byte_count, &n);
>> +    n += size;
>> +    atomic_store_relaxed(&netdev_flow->stats.byte_count, n);
>> +
>> +    atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags);
>> +    flags |= tcp_flags;
>> +    atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags);
>> }
>> 
>> static void
>> dp_netdev_count_packet(struct dp_netdev_pmd_thread *pmd,
>>                        enum dp_stat_type type, int cnt)
>> {
>> -    pmd->stats.n[type] += cnt;
>> +    unsigned long long n;
>> +
>> +    atomic_read_relaxed(&pmd->stats.n[type], &n);
>> +    n += cnt;
>> +    atomic_store_relaxed(&pmd->stats.n[type], n);
>> }
>> 
>> static int
>> --
>> 2.1.4
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=ABxEcTwcHFc-s6Ygmxu3gn9QHlT6aZ9Eak3R5sgYo3E&s=OXtA3NGPp5BEptVVCzwBTcXOvgjJPqSX5eufmTZSNQw&e= 




More information about the dev mailing list