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

Stokes, Ian ian.stokes at intel.com
Thu Jun 24 13:20:29 UTC 2021


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

Hi Amber/Harry,

Thanks for the patch, a few comments inline below.

> 
> 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>
> ---
>  lib/automake.mk                   |   1 +
>  lib/dpif-netdev-extract-study.c   | 119 ++++++++++++++++++++++++++++++
>  lib/dpif-netdev-private-extract.c |   5 ++
>  lib/dpif-netdev-private-extract.h |  14 +++-
>  4 files changed, 138 insertions(+), 1 deletion(-)
>  create mode 100644 lib/dpif-netdev-extract-study.c
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 6657b9ae5..3080bb04a 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -114,6 +114,7 @@ lib_libopenvswitch_la_SOURCES = \
>  	lib/dpif-netdev.c \
>  	lib/dpif-netdev.h \
>  	lib/dpif-netdev-private-dfc.c \
> +	lib/dpif-netdev-extract-study.c \
Headers should be added alphabetically.

>  	lib/dpif-netdev-private-dfc.h \
>  	lib/dpif-netdev-private-dpcls.h \
>  	lib/dpif-netdev-private-dpif.c \
> diff --git a/lib/dpif-netdev-extract-study.c b/lib/dpif-netdev-extract-study.c
> new file mode 100644
> index 000000000..d063d040c
> --- /dev/null
> +++ b/lib/dpif-netdev-extract-study.c
> @@ -0,0 +1,119 @@
> +/*
> + * Copyright (c) 2021 Intel.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "dpif-netdev-private-extract.h"
> +#include "dpif-netdev-private-thread.h"
> +#include "openvswitch/vlog.h"
> +#include "ovs-thread.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dpif_mfex_extract_study);
> +
> +/* Max size of packets to be compared. */
> +#define MFEX_MAX_COUNT (128)
> +
> +/* This value is the threshold for the amount of packets that
> + * must hit on the optimized miniflow extract before it will be
> + * accepted and used in the datapath after the study phase. */
> +#define MFEX_MIN_HIT_COUNT_FOR_USE (MFEX_MAX_COUNT / 2)
> +
> +/* Struct to hold miniflow study stats. */
> +struct study_stats {
> +    uint32_t pkt_count;
> +    uint32_t impl_hitcount[MFEX_IMPLS_MAX_SIZE];
> +};
> +
> +/* Define per thread data to hold the study stats. */
> +DEFINE_PER_THREAD_MALLOCED_DATA(struct study_stats *, study_stats);
> +
> +/* Allocate per thread PMD pointer space for study_stats. */
> +static inline struct study_stats *
> +get_study_stats(void)

Would maybe suggest a name change here, get_study_stats sounds as if info is being returned whereas whats actually happening is that the memory for the stats are being provisioned.
> +{
> +    struct study_stats *stats = study_stats_get();
> +    if (OVS_UNLIKELY(!stats)) {
> +       stats = xzalloc(sizeof *stats);
> +       study_stats_set_unsafe(stats);
Can you explain why above is set unsafe? Where does that function originate from?

> +    }
> +    return stats;
> +}
> +
> +uint32_t
> +mfex_study_traffic(struct dp_packet_batch *packets,
> +                   struct netdev_flow_key *keys,
> +                   uint32_t keys_size, odp_port_t in_port,
> +                   void *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;
> +    uint32_t impl_count = dpif_miniflow_extract_info_get(&miniflow_funcs);
> +    struct study_stats *stats = get_study_stats();
> +
> +    /* 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 the comment above would prefer to keep with the OVS coding style and close comment vertically aligned.

https://docs.openvswitch.org/en/latest/internals/contributing/coding-style/#comments

/*
 *
 */

> +    for (int i = MFEX_IMPL_START_IDX; i < impl_count; i++) {
> +        if (miniflow_funcs[i].available) {
> +            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 than we dont overwrite the keys
Typo above than -> then
> +             * 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_COUNT) {
> +        uint32_t best_func_index = MFEX_IMPL_START_IDX;
> +        uint32_t max_hits = 0;
> +        for (int i = MFEX_IMPL_START_IDX; i < impl_count; i++) {
> +            if (stats->impl_hitcount[i] > max_hits) {
> +                max_hits = stats->impl_hitcount[i];
> +                best_func_index = i;
> +            }
> +        }
> +
> +        if (max_hits >= MFEX_MIN_HIT_COUNT_FOR_USE) {
> +            /* Set the implementation to index with max_hits. */
> +            pmd->miniflow_extract_opt =
> +                        miniflow_funcs[best_func_index].extract_func;
> +            VLOG_INFO("MFEX study chose impl %s: (hits %d/%d pkts)\n",
> +                      miniflow_funcs[best_func_index].name, max_hits,
> +                      stats->pkt_count);
> +        } else {
> +            /* Set the implementation to null for default miniflow. */
> +            pmd->miniflow_extract_opt = NULL;
> +            VLOG_INFO("Not enough packets matched (%d/%d), disabling"
> +                      " optimized MFEX.\n", max_hits, stats->pkt_count);
> +        }
> +        /* Reset stats so that study function can be called again
> +         * for next traffic type and optimal function ptr can be
> +         * choosen. */

Typo, chosen -> chosen.

BR
Ian

> +        memset(stats, 0, sizeof(struct study_stats));
> +    }
> +    return mask;
> +}
> diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c
> index 0741c19f9..d86268a1d 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -42,6 +42,11 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = {
>          .extract_func = NULL,
>          .name = "disable",
>      },
> +    {
> +        .probe = NULL,
> +        .extract_func = mfex_study_traffic,
> +        .name = "study",
> +    },
>  };
> 
>  BUILD_ASSERT_DECL(MFEX_IMPLS_MAX_SIZE > ARRAY_SIZE(mfex_impls));
> diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-extract.h
> index 455a7b590..3ada413bb 100644
> --- a/lib/dpif-netdev-private-extract.h
> +++ b/lib/dpif-netdev-private-extract.h
> @@ -27,7 +27,7 @@
>  /* Skip the autovalidator study and null when iterating all available
>   * miniflow implementations.
>   */
> -#define MFEX_IMPL_START_IDX (1)
> +#define MFEX_IMPL_START_IDX (3)
> 
>  /* Forward declarations. */
>  struct dp_packet;
> @@ -106,4 +106,16 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *batch,
>                                      uint32_t keys_size, odp_port_t in_port,
>                                      void *pmd_handle);
> 
> +/* Retrieve the number of packets by studying packets using different miniflow
> + * implementations to choose the best implementation using the maximum
> hitmask
> + * count.
> + * On error, returns a zero for no packets.
> + * On success, returns mask of the packets hit.
> + */
> +uint32_t
> +mfex_study_traffic(struct dp_packet_batch *packets,
> +                   struct netdev_flow_key *keys,
> +                   uint32_t keys_size, odp_port_t in_port,
> +                   void *pmd_handle);
> +
>  #endif /* DPIF_NETDEV_AVX512_EXTRACT */
> --
> 2.25.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list