> On 25 Feb 2015, at 19:08, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
>
>> On Feb 25, 2015, at 8:43 AM, Daniele Di Proietto <diproiet...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
