[ovs-dev] [v10 03/12] dpif-netdev: Add study function to select the best mfex function

[Amber, Kumar]

Hi Eelco,

Fixed both and hence keeping the Ack 😊

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Tuesday, July 13, 2021 2:58 PM
> To: Amber, Kumar <kumar.amber at intel.com>
> Cc: ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; Van
> Haaren, Harry <harry.van.haaren at intel.com>; Ferriter, Cian
> <cian.ferriter at intel.com>; Stokes, Ian <ian.stokes at intel.com>
> Subject: Re: [v10 03/12] dpif-netdev: Add study function to select the best mfex
> function
> 
> 
> 
> On 13 Jul 2021, at 7:32, Kumar Amber wrote:
> 
> > The study function runs all the available implementations of
> > miniflow_extract and makes a choice whose hitmask has maximum hits and
> > sets the mfex to that function.
> >
> > Study can be run at runtime using the following command:
> >
> > $ ovs-appctl dpif-netdev/miniflow-parser-set study
> >
> > Signed-off-by: Kumar Amber <kumar.amber at intel.com>
> > Co-authored-by: Harry van Haaren <harry.van.haaren at intel.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> > Acked-by: Eelco Chaudron <echaudro at redhat.com>
> >
> 
> I did ack the previous patch by accident, See some more comments below, if
> fixed in v11 keep my ack.
> 
> > ---
> > v10:
> > - fix minor comments from Eelco
> > v9:
> > - fix comments Flavio
> > v8:
> > - fix review comments Flavio
> > v7:
> > - fix review comments(Eelco)
> > v5:
> 
> <SNIP>
> 
> > +uint32_t
> > +mfex_study_traffic(struct dp_packet_batch *packets,
> > +                   struct netdev_flow_key *keys,
> > +                   uint32_t keys_size, odp_port_t in_port,
> > +                   struct dp_netdev_pmd_thread *pmd_handle) {
> > +    uint32_t hitmask = 0;
> > +    uint32_t mask = 0;
> > +    struct dp_netdev_pmd_thread *pmd = pmd_handle;
> > +    struct dpif_miniflow_extract_impl *miniflow_funcs;
> > +    struct study_stats *stats = mfex_study_get_study_stats_ptr();
> > +    miniflow_funcs = dpif_mfex_impl_info_get();
> > +
> > +    /* Run traffic optimized miniflow_extract to collect the hitmask
> > +     * to be compared after certain packets have been hit to choose
> > +     * the best miniflow_extract version for that traffic.
> > +     */
> > +    for (int i = MFEX_IMPL_START_IDX; i < MFEX_IMPL_MAX; i++) {
> > +        if (!miniflow_funcs[i].available) {
> > +            continue;
> > +        }
> > +
> > +        hitmask = miniflow_funcs[i].extract_func(packets, keys, keys_size,
> > +                                                 in_port, pmd_handle);
> > +        stats->impl_hitcount[i] += count_1bits(hitmask);
> > +
> > +        /* If traffic is not classified then we dont overwrite the keys
> > +         * array in minfiflow implementations so its safe to create a
> > +         * mask for all those packets whose miniflow have been created.
> > +         */
> > +        mask |= hitmask;
> > +    }
> > +
> > +    stats->pkt_count += dp_packet_batch_size(packets);
> > +
> > +    /* Choose the best implementation after a minimum packets have been
> > +     * processed.
> > +     */
> > +    if (stats->pkt_count >= MFEX_MAX_PKT_COUNT) {
> > +        uint32_t best_func_index = MFEX_IMPL_START_IDX;
> > +        uint32_t max_hits = 0;
> > +        for (int i = MFEX_IMPL_START_IDX; i < MFEX_IMPL_MAX; i++) {
> > +            if (stats->impl_hitcount[i] > max_hits) {
> > +                max_hits = stats->impl_hitcount[i];
> > +                best_func_index = i;
> > +            }
> > +        }
> > +
> > +        /* If 50% of the packets hit, enable the function. */
> > +        if (max_hits >= (mfex_study_pkts_count / 2)) {
> > +            miniflow_extract_func mf_func =
> > +                        miniflow_funcs[best_func_index].extract_func;
> > +            atomic_uintptr_t *pmd_func = (void *)&pmd->miniflow_extract_opt;
> > +            atomic_store_relaxed(pmd_func, (uintptr_t) mf_func);
> > +            VLOG_INFO("MFEX study chose impl %s: (hits %u/%u pkts)",
> > +                      miniflow_funcs[best_func_index].name, max_hits,
> > +                      stats->pkt_count);
> 
> You only change the %d/%d here, but there are two more occurrences below.
> 
> > +        } else {
> > +            /* Set the implementation to null for default miniflow. */
> > +            miniflow_extract_func mf_func =
> > +                        miniflow_funcs[MFEX_IMPL_SCALAR].extract_func;
> > +            atomic_uintptr_t *pmd_func = (void *)&pmd->miniflow_extract_opt;
> > +            atomic_store_relaxed(pmd_func, (uintptr_t) mf_func);
> > +            VLOG_INFO("Not enough packets matched (%d/%d), disabling"
> > +                      " optimized MFEX.", max_hits, stats->pkt_count);
> > +        }
> > +
> > +        /* In debug mode show stats for all the counters. */
> > +        if (VLOG_IS_DBG_ENABLED()) {
> > +
> > +            for (int i = MFEX_IMPL_START_IDX; i < MFEX_IMPL_MAX; i++) {
> > +                VLOG_DBG("MFEX study results for implementation %s:"
> > +                         " (hits %d/%d pkts)",
> > +                         miniflow_funcs[i].name, stats->impl_hitcount[i],
> > +                         stats->pkt_count);
> > +            }
> > +        }
> > +
> 
> <SNIP>