[ovs-dev] [PATCH 5/5] dpif-netdev: Add dpif-netdev/pmd-stats-* appctl commands

Daniele Di Proietto diproiettod at vmware.com
Thu Feb 26 15:07:03 UTC 2015


> On 25 Feb 2015, at 19:08, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> 
> 
>> On Feb 25, 2015, at 8:43 AM, Daniele Di Proietto <diproiettod at vmware.com> wrote:
>> 
>> 
> 
> (snip)
> 
>> 
>> +enum pmd_info_type {
>> +    PMD_INFO_SHOW_STATS,  /* show how cpu cycles are spent */
>> +    PMD_INFO_CLEAR_STATS  /* set the cycles count to 0 */
>> +};
>> +
>> +static void
>> +dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[],
>> +                     void *aux)
>> +{
>> +    struct ds reply = DS_EMPTY_INITIALIZER;
>> +    struct dp_netdev_pmd_thread *pmd;
>> +    struct dp_netdev *dp = NULL;
>> +    enum pmd_info_type type = (enum pmd_info_type) aux;
>> +
>> +    ovs_mutex_lock(&dp_netdev_mutex);
>> +
>> +    if (argc == 2) {
>> +        dp = shash_find_data(&dp_netdevs, argv[1]);
>> +    } else if (shash_count(&dp_netdevs) == 1) {
>> +        /* There's only one datapath */
>> +        dp = shash_random_node(&dp_netdevs)->data;
>> +    }
>> +
>> +    if (!dp) {
>> +        ovs_mutex_unlock(&dp_netdev_mutex);
>> +        unixctl_command_reply_error(conn,
>> +                                    "please specify an existing datapath");
>> +        return;
>> +    }
>> +
>> +    CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>> +        struct dp_netdev_pmd_stats s;
>> +        struct dp_netdev_pmd_cycles t;
>> +        unsigned long long total_packets = 0;
>> +        uint64_t total_cycles = 0;
>> +        int i;
>> +
>> +        if (type == PMD_INFO_CLEAR_STATS) {
>> +            /* We cannot write 'stats' and 'cycles' (because they're written
>> +             * by other threads) and we shouldn't change 'stats' (because
>> +             * ther're used to count datapath stats, which must not be cleared
>> +             * here).
>> +             * Insted, we save the current values and subtract them from the
> 
> “instead”

Oops, I’ll fix that

> 
>> +             * values to be displayed in the future */
>> +            pmd->cycles_zero = pmd->cycles;
>> +            pmd->stats_zero = pmd->stats;
> 
> It seems to me that this access should be protected with a similar mechanism we use in cmaps, as any concurrent write by the pmd thread may result the “zero” versions to be inconsistent, a mixture of the old and new. Or is it the case that this does not matter? I guess that the cycles and stats need not be consistent with each other, though.
> 

I’ve thought of that, but I didn’t want to add much overhead to the pmd threads. I concluded it was fine to “sync” on RCU quiesce only. We’re doing the same in dpif_netdev_get_stats() after all. If you think it’s ok I would prefer to leave it as it is. I’m waiting for your feedback

>> +            continue;
>> +        }
>> +        t = pmd->cycles;
>> +        s = pmd->stats;
>> +
>> +        for (i = 0; i < ARRAY_SIZE(t.n); i++) {
>> +            t.n[i] -= pmd->cycles_zero.n[i];
>> +            total_cycles += t.n[i];
>> +        }
>> +        for (i = 0; i < ARRAY_SIZE(s.n); i++) {
>> +            s.n[i] -= pmd->stats_zero.n[i];
>> +            if (i != DP_STAT_LOST) {
>> +                total_packets += s.n[i];
>> +            }
>> +        }
>> +
> 
> Similar concern up here.

The *_zero members are read and written only by the main thread (the one that handles the appctl command). There’s even the dp_netdev_mutex to enforce that.

Thanks,

Daniele



More information about the dev mailing list