> On 30 Mar 2015, at 22:41, Ethan Jackson <et...@nicira.com> wrote:
>
> You're not going to like my review of this patch =)
>
Anything to try to make the code better :-)
> So the code is very high quality, though I haven't read it super
> closely yet. My main question, is if this is really the right
> approach to handle stats in the DPDK fast path in general.
>
> So here's my question: why not just use atomic variables for the
> members of dp_netdev_flow_stats and dp_netdev_pmd_stats? If you use
> relaxed reads and writes (no increments), then there's zero
> performance penalty on x86. Also it has the added side benefits of
> not requiring additional error-prone multiplatform compat code, saves
> us having to expose the seqlock, and makes it impossible to access the
> variables without locking properly. All said, we get the exact same
> performance profile of this proposed series, with significantly
> simpler and less error-prone code.
>
> What do you think?
>
You’re right, it makes maintenance easier. Thanks for the suggestion!
I’m sending another version of the patch with atomic statistics.
Minor issue: the compilers I’ve tried (GCC 4.9, 4.8 clang 3.5 and sparse
0.4.5) do not report a warning if I try to access atomic variables without
atomic function. Maybe someone can confirm this?
> Ethan
>
>
> On Fri, Mar 27, 2015 at 9:29 AM, Daniele Di Proietto
> <diproiet...@vmware.com> wrote:
>> While the values are written only by a single thread, reading them
>> concurrently might produce incorrect results, given the lack of 64-bit
>> atomic loads on 32-bit systems.
>>
>> This fix prevent unconsistent reads of 64-bit values on 32-bit systems.
>>
>> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
>> ---
>> lib/dpif-netdev.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 2637e8d..ba677eb 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -64,6 +64,7 @@
>> #include "sset.h"
>> #include "timeval.h"
>> #include "tnl-arp-cache.h"
>> +#include "u64-stats-sync.h"
>> #include "unixctl.h"
>> #include "util.h"
>> #include "openvswitch/vlog.h"
>> @@ -301,6 +302,9 @@ struct dp_netdev_flow {
>>
>> /* Statistics. */
>> struct dp_netdev_flow_stats stats;
>> + /* Used to protect 'stats'. Only guarantees consistency of single stats
>> + * members, not of the structure as a whole */
>> + struct u64_stats_sync stats_lock;
>>
>> /* Actions. */
>> OVSRCU_TYPE(struct dp_netdev_actions *) actions;
>> @@ -382,6 +386,9 @@ struct dp_netdev_pmd_thread {
>>
>> /* Statistics. */
>> struct dp_netdev_pmd_stats stats;
>> + /* Used to protect 'stats'. Only guarantees consistency of single stats
>> + * members, not of the structure as a whole */
>> + struct u64_stats_sync stats_lock;
>>
>> struct latch exit_latch; /* For terminating the pmd thread. */
>> atomic_uint change_seq; /* For reloading pmd ports. */
>> @@ -754,10 +761,19 @@ 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) {
>> + struct dp_netdev_pmd_stats s;
>> + uint32_t c;
>> +
>> + do {
>> + c = u64_stats_read_begin(&pmd->stats_lock);
>> + s = pmd->stats;
>> + } while (!u64_stats_read_correct(&pmd->stats_lock, c));
>> +
>> 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];
>> +
>> + stats->n_hit += s.n[DP_STAT_HIT];
>> + stats->n_missed += s.n[DP_STAT_MISS];
>> + stats->n_lost += s.n[DP_STAT_LOST];
>> }
>> stats->n_masks = UINT32_MAX;
>> stats->n_mask_hit = UINT64_MAX;
>> @@ -1548,10 +1564,15 @@ static void
>> 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;
>> + uint32_t c;
>> + do {
>> + c = u64_stats_read_begin(&netdev_flow->stats_lock);
>> +
>> + 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;
>> + } while (!u64_stats_read_correct(&netdev_flow->stats_lock, c));
>> }
>>
>> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for
>> @@ -1735,6 +1756,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>> /* Do not allocate extra space. */
>> flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len);
>> memset(&flow->stats, 0, sizeof flow->stats);
>> + memset(&flow->stats_lock, 0, sizeof flow->stats_lock);
>> flow->dead = false;
>> *CONST_CAST(int *, &flow->pmd_id) = pmd->core_id;
>> *CONST_CAST(struct flow *, &flow->flow) = match->flow;
>> @@ -2670,18 +2692,30 @@ dp_netdev_flow_used(struct dp_netdev_flow
>> *netdev_flow, int cnt, int size,
>> uint16_t tcp_flags)
>> {
>> long long int now = time_msec();
>> + uint32_t c;
>> +
>> + c = u64_stats_write_begin(&netdev_flow->stats_lock);
>>
>> 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;
>> +
>> + u64_stats_write_end(&netdev_flow->stats_lock, c);
>> }
>>
>> static void
>> dp_netdev_count_packet(struct dp_netdev_pmd_thread *pmd,
>> enum dp_stat_type type, int cnt)
>> {
>> + uint32_t c;
>> +
>> + if (!cnt)
>> + return;
>> +
>> + c = u64_stats_write_begin(&pmd->stats_lock);
>> pmd->stats.n[type] += cnt;
>> + u64_stats_write_end(&pmd->stats_lock, c);
>> }
>>
>> static int
>> --
>> 2.1.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@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=F7XZgy4UdNNeJnxr1xwBXFMFZNDwd3ftwiI8xVoyWgg&s=8EWnbDvB_nGnZnRIozFlOhBRkhlDf2WKZo1ovTx1CH8&e=
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
