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

Jarno Rajahalme jrajahalme at nicira.com
Thu Feb 26 18:08:57 UTC 2015


> On Feb 26, 2015, at 7:07 AM, Daniele Di Proietto <diproiettod at vmware.com> wrote:
> 
>> 
>> 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
> 

I do not know what you mean by “sync on RCU quiesce only”. For what I can see the dip_netdev_get_stats() has the same problem, but this affects only 32-bit compiled OVS, as on 64-bit systems the access to the properly aligned 64-bit integers should be atomic anyway. Maybe add overheads for 32-bit builds only?

I would also be fine with us supporting userspace datapath/DPDK only for 64-bit builds.

>>> +            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.

Oops, I missed that, thanks!

  Jarno

> 
> Thanks,
> 
> Daniele




More information about the dev mailing list