[ovs-dev] [PATCH v4 1/3] dpif-netdev: Refactor PMD performance into dpif-netdev-perf

[Jan Scheurich]

> >     > Should this have some additional cover for the ENOSPC case?  Meaning, we
> >     > will now increment a failure condition, but I'm not sure it should be
> >     > considered an 'upcall' failure?  After all, we'll still perform the actions.
> >
> >     Good question. I inherited this change from Darrell's original patch set.
> >
> >     Looking at ofproto, there are a number of different conditions that return an ENOSPC error from the upcall: reached ukey limit,
> recirculation without matching recirc_ref, ukey installation failed, possibly more. I guess the common denominator is that we execute the
> dp actions for the packet just without installing a datapath flow. So we can consider the upcall as successful. I suggest to return zero when
> we get to the end of the function as the only error possible at that stage is ENOSPC.
> >
> >     @Darrell. OK with you?
> >
> >
> > [Darrell] No, I don’t think we can just translate an upcall that fails to install a flow due to lack of space as ‘success/ok’.
> >                One of 2 main purposes of a miss upcall is to install a flow in the fast path and I think that is also the common expectation of
> success.
> >                The problem may also be persistent or even as bug.
> >
> >                Two possible options are: to keep the upcall_fail_cnt++ increment or to differentiate the 2 cases as something like -
> >                upcall_fail_to_install_flow and just plain upcall_fail_cnt and refactor accordingly.
> >
> 
> I prefer the second case.  In that instance, it seems as though the
> error condition isn't overloaded.

I would like to decouple this discussion, so that it doesn't block the present patch series. I suggest to leave it as it is in v4 for now.

To me the meaning of the various counters is rather ambiguous. The original requirement defined in dpif-provider.h looks as follows:

/* Statistics for a dpif as a whole. */
struct dpif_dp_stats {
    uint64_t n_hit;             /* Number of flow table matches. */
    uint64_t n_missed;          /* Number of flow table misses. */
    uint64_t n_lost;            /* Number of misses not sent to userspace. */
    uint64_t n_flows;           /* Number of flows present. */
    uint64_t n_mask_hit;        /* Number of mega flow masks visited for
                                   flow table matches. */
    uint32_t n_masks;           /* Number of mega flow masks. */
};

The current mapping in dpif-netdev.c does not match that in many ways:

static int
dpif_netdev_get_stats(const struct dpif *dpif, struct dpif_dp_stats *stats)
{
    struct dp_netdev *dp = get_dp_netdev(dpif);
    struct dp_netdev_pmd_thread *pmd;
    uint64_t pmd_stats[PMD_N_STATS];

    stats->n_flows = stats->n_hit =
            stats->n_mask_hit = stats->n_missed = stats->n_lost = 0;
    CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
        stats->n_flows += cmap_count(&pmd->flow_table);
        pmd_perf_read_counters(&pmd->perf_stats, pmd_stats);
        stats->n_hit += pmd_stats[PMD_STAT_EXACT_HIT];
        stats->n_mask_hit += pmd_stats[PMD_STAT_MASKED_HIT];
        stats->n_missed += pmd_stats[PMD_STAT_MISS];
        stats->n_lost += pmd_stats[PMD_STAT_LOST];
    }
    stats->n_masks = UINT32_MAX;

    return 0;
}

We report EMC hits as "flow table matches", DPLCS hits as "Number of mega flow masks visited for flow table matches" and the number of failed upcalls as " Number of misses not sent to userspace".

I briefly checked the kernel datapath and they seem to really report things as intended by the dpif-provider.h spec.

So let’s discuss and tidy this up in a separate thread.