Hi Flavio, > -----Original Message----- > From: Flavio Leitner <[email protected]> > Sent: Sunday, July 11, 2021 6:14 AM > To: Amber, Kumar <[email protected]> > Cc: [email protected]; [email protected] > Subject: Re: [ovs-dev] [v8 03/12] dpif-netdev: Add study function to select > the > best mfex function > > > Hi, > > On Fri, Jul 09, 2021 at 05:35:53PM +0530, kumar Amber wrote: > > From: Kumar Amber <[email protected]> > > > > 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 <[email protected]> > > Co-authored-by: Harry van Haaren <[email protected]> > > Signed-off-by: Harry van Haaren <[email protected]> > > > > --- > > v7: > > - fix review comments(Eelco) > > v5: > > - fix review comments(Ian, Flavio, Eelco) > > - add Atomic set in study > > --- > > --- > > NEWS | 3 + > > lib/automake.mk | 1 + > > lib/dpif-netdev-extract-study.c | 143 ++++++++++++++++++++++++++++++ > > lib/dpif-netdev-private-extract.c | 17 ++++ > > lib/dpif-netdev-private-extract.h | 20 +++++ > > 5 files changed, 184 insertions(+) > > create mode 100644 lib/dpif-netdev-extract-study.c > > > > diff --git a/NEWS b/NEWS > > index e8b4e0405..95bf386e3 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -31,6 +31,9 @@ Post-v2.15.0 > > * Add command line option to switch between mfex function pointers. > > * Add miniflow extract auto-validator function to compare different > > miniflow extract implementations against default implementation. > > + * Add study function to miniflow function table which studies > > + packet > > I guess that is not indented correctly. >
Fixed. > > + and automatically chooses the best miniflow implementation for that > > + traffic. > > - ovs-ctl: > > * New option '--no-record-hostname' to disable hostname configuration > > in ovsdb on startup. > > diff --git a/lib/automake.mk b/lib/automake.mk index > > 53b8abc0f..f4f36325e 100644 > > --- a/lib/automake.mk > > +++ b/lib/automake.mk > > @@ -107,6 +107,7 @@ lib_libopenvswitch_la_SOURCES = \ > > lib/dp-packet.h \ > > lib/dp-packet.c \ > > lib/dpdk.h \ > > + lib/dpif-netdev-extract-study.c \ > > lib/dpif-netdev-lookup.h \ > > lib/dpif-netdev-lookup.c \ > > lib/dpif-netdev-lookup-autovalidator.c \ diff --git > > a/lib/dpif-netdev-extract-study.c b/lib/dpif-netdev-extract-study.c > > new file mode 100644 index 000000000..f14464b2b > > --- /dev/null > > +++ b/lib/dpif-netdev-extract-study.c > > @@ -0,0 +1,143 @@ > > +/* > > + * 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-thread.h" > > +#include "openvswitch/vlog.h" > > +#include "ovs-thread.h" > > + > > +VLOG_DEFINE_THIS_MODULE(dpif_mfex_extract_study); > > + > > +/* Max count of packets to be compared. */ #define MFEX_MAX_COUNT > > +(128) > > + > > +static uint32_t mfex_study_pkts_count = 0; > > + > > +/* Struct to hold miniflow study stats. */ struct study_stats { > > + uint32_t pkt_count; > > + uint32_t impl_hitcount[MFEX_IMPL_MAX]; }; > > + > > +/* 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 * > > +mfex_study_get_study_stats_ptr(void) > > +{ > > + struct study_stats *stats = study_stats_get(); > > + if (OVS_UNLIKELY(!stats)) { > > + stats = xzalloc(sizeof *stats); > > + study_stats_set_unsafe(stats); > > + } > > + 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, > > + 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(); > > + uint32_t impl_count = dpif_mfex_impl_info_get(&miniflow_funcs); > > This module has access to enum and the MFEX_IMPL_MAX should be enough. > See more details in dpif_mfex_impl_info_get() below. > Yes cleaned and reworked this part of code. > > + > > + if (impl_count <= 0) { > > + return 0; > > + } > > Not sure how that can happen because at least there will one implementation > for scalar and another for study. If that is not enough we can build assert > because it doesn't change in runtime. > Cleaned and reworked this part in v9. > > + > > + /* 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 < impl_count; i++) { > > Use MFEX_IMPL_MAX instead of impl_count. > Fixed. > > + 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_COUNT) { > > MFEX_MAX_PKT_COUNT please. > > Fixed. > > + 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 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 %d/%d pkts)", > > + miniflow_funcs[best_func_index].name, max_hits, > > + stats->pkt_count); > > + } 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 < impl_count; i++) { > > + VLOG_DBG("MFEX study results for implementation %s:" > > + " (hits %d/%d pkts)", > > + miniflow_funcs[i].name, stats->impl_hitcount[i], > > + stats->pkt_count); > > + } > > + } > > + > > + /* Reset stats so that study function can be called again > > + * for next traffic type and optimal function ptr can be > > + * chosen. > > + */ > > + 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 2cc1bc9d5..bb7c98f31 100644 > > --- a/lib/dpif-netdev-private-extract.c > > +++ b/lib/dpif-netdev-private-extract.c > > @@ -47,6 +47,11 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = > { > > .probe = NULL, > > .extract_func = NULL, > > .name = "scalar", }, > > + > > + [MFEX_IMPL_STUDY] = { > > + .probe = NULL, > > + .extract_func = mfex_study_traffic, > > + .name = "study", }, > > }; > > > > BUILD_ASSERT_DECL(MFEX_IMPL_MAX >= ARRAY_SIZE(mfex_impls)); @@ - > 162,6 > > +167,18 @@ dp_mfex_impl_get_by_name(const char *name, > miniflow_extract_func *out_func) > > return -ENOENT; > > } > > > > +int > > +dpif_mfex_impl_info_get(struct dpif_miniflow_extract_impl **out_ptr) > > +{ > > Why not just use like below? > struct dpif_miniflow_extract_impl * > dpif_mfex_impl_info_get(void) { > return mfex_impls; > } > The size is always MFEX_IMPL_MAX. > > Fixed and reworked. > > > + if (out_ptr == NULL) { > > + return -EINVAL; > > + } > > + > > + *out_ptr = mfex_impls; > > + > > + return ARRAY_SIZE(mfex_impls); > > +} > > + > > uint32_t > > dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, > > struct netdev_flow_key *keys, > > diff --git a/lib/dpif-netdev-private-extract.h > > b/lib/dpif-netdev-private-extract.h > > index 062a10648..802496cb9 100644 > > --- a/lib/dpif-netdev-private-extract.h > > +++ b/lib/dpif-netdev-private-extract.h > > @@ -76,6 +76,7 @@ struct dpif_miniflow_extract_impl { enum > > dpif_miniflow_extract_impl_idx { > > MFEX_IMPL_AUTOVALIDATOR, > > MFEX_IMPL_SCALAR, > > + MFEX_IMPL_STUDY, > > MFEX_IMPL_MAX > > }; > > > > @@ -105,6 +106,13 @@ miniflow_extract_func > > dp_mfex_impl_get_default(void); > > /* Overrides the default MFEX with the user set MFEX. */ int > > dp_mfex_impl_set_default_by_name(const char *name); > > > > +/* Retrieve the array of miniflow implementations for iteration. > > + * On error, returns a negative number. > > + * On success, returns the size of the arrays pointed to by the out > > parameter. > > + */ > > +int > > +dpif_mfex_impl_info_get(struct dpif_miniflow_extract_impl **out_ptr); > > + > > > > /* Initializes the available miniflow extract implementations by probing > > for > > * the CPU ISA requirements. As the runtime available CPU ISA does > > not change @@ -126,4 +134,16 @@ > dpif_miniflow_extract_autovalidator(struct dp_packet_batch *batch, > > uint32_t keys_size, odp_port_t in_port, > > struct dp_netdev_pmd_thread > > *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, > > + struct dp_netdev_pmd_thread *pmd_handle); > > + > > #endif /* MFEX_AVX512_EXTRACT */ > > -- > > 2.25.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
