[ovs-dev] [v4 03/12] dpif-netdev: Add study function to select the best mfex function
Amber, Kumar
kumar.amber at intel.com
Thu Jun 24 14:39:33 UTC 2021
Hi Ian ,
Thanks Again, replies are inline.
<Snipped>
> > @@ -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.
>
Fixed in v5.
> > 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.
Fixed in v5. Renamed to get_study_stats_ptr().
> > +{
> > + 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
Fixed at all the places in v5.
>
> /*
> *
> */
>
> > + 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
Fixed in v5.
> > + * 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.
Fixed in v5.
>
> 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