Hi Flavio, Thanks Again, replies are inline.
> -----Original Message----- > From: Flavio Leitner <[email protected]> > Sent: Monday, June 28, 2021 8:26 AM > To: Amber, Kumar <[email protected]> > Cc: [email protected]; [email protected] > Subject: Re: [ovs-dev] [v4 06/12] dpif-netdev: Add additional packet count > parameter for study function > > > Hi, > > On Thu, Jun 17, 2021 at 09:57:48PM +0530, Kumar Amber wrote: > > This commit introduces additonal command line paramter > ^^^^^^^^ ^^^^^^^^ > Typos. > Fixed in v5. > > for mfex study function. If user provides additional packet out it is > > used in study to compare minimum packets which must be processed else > > a default value is choosen. > > > > $ OVS_DIR/utilities/ovs-appctl dpif-netdev/miniflow-parser-set study > > 500 > > There is no need to include "OVS_DIR/utilities/" as it depends on each > particular deployment. > Removed. > > > > Signed-off-by: Kumar Amber <[email protected]> > > --- > > Documentation/topics/dpdk/bridge.rst | 8 ++++++- > > lib/dpif-netdev-extract-study.c | 15 +++++++++++- > > lib/dpif-netdev-private-extract.h | 8 +++++++ > > lib/dpif-netdev.c | 34 +++++++++++++++++++++++----- > > 4 files changed, 57 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/topics/dpdk/bridge.rst > > b/Documentation/topics/dpdk/bridge.rst > > index 1c78adc75..e7e91289a 100644 > > --- a/Documentation/topics/dpdk/bridge.rst > > +++ b/Documentation/topics/dpdk/bridge.rst > > @@ -288,7 +288,13 @@ An implementation can be selected manually by > the following command :: > > Also user can select the study implementation which studies the > > traffic for a specific number of packets by applying all availbale > > implementaions of miniflow extract and than chooses the one with most > > optimal result for that -traffic pattern. > > +traffic pattern. User can also provide additonal parameter as packet > > +count which is minimum packets which OVS must study before choosing > > +optimal implementation, If no packet count is provided than default > value is choosen. > > + > > +Study can be selected with packet count by the following command :: > > + > > + $ ovs-appctl dpif-netdev/miniflow-parser-set study 1024 > > Ian already commented here. > > Fixed. > > > > Miniflow Extract Validation > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/lib/dpif-netdev-extract-study.c > > b/lib/dpif-netdev-extract-study.c index d063d040c..c48fb125e 100644 > > --- a/lib/dpif-netdev-extract-study.c > > +++ b/lib/dpif-netdev-extract-study.c > > @@ -55,6 +55,19 @@ get_study_stats(void) > > return stats; > > } > > > > +static uint32_t pkt_compare_count = 0; > > If we had a convention as mentioned in patch #3, this could be > mfex_study_compare_count or maybe mfex_study_pkts_count. > Changed to mfex_study_pkts_count In v5. > > + > > +uint32_t mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, > > + struct dpif_miniflow_extract_impl > > +*opt) { > > + if ((opt->extract_func == mfex_study_traffic) && (pkt_cmp_count != 0)) > { > > + pkt_compare_count = pkt_cmp_count; > > + return 0; > > + } > > + pkt_compare_count = MFEX_MAX_COUNT; > > + return -EINVAL; > > The documentation is not here, so not sure if it's missing or if there will > be a > patch updating the man-page at least. My suggestion would be to allow 0 to > reset to built-in default. > Added a Documentation explaining the function in .h. > > +} > > + > > uint32_t > > mfex_study_traffic(struct dp_packet_batch *packets, > > struct netdev_flow_key *keys, @@ -87,7 +100,7 @@ > > mfex_study_traffic(struct dp_packet_batch *packets, > > > > /* Choose the best implementation after a minimum packets have been > > * processed. */ > > - if (stats->pkt_count >= MFEX_MAX_COUNT) { > > + if (stats->pkt_count >= pkt_compare_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++) { diff > > --git a/lib/dpif-netdev-private-extract.h > > b/lib/dpif-netdev-private-extract.h > > index d8a284db7..0ec74bef9 100644 > > --- a/lib/dpif-netdev-private-extract.h > > +++ b/lib/dpif-netdev-private-extract.h > > @@ -127,5 +127,13 @@ dpif_miniflow_extract_get_default(void); > > * overridden at runtime. */ > > void > > dpif_miniflow_extract_set_default(miniflow_extract_func func); > > +/* Sets the packet count from user to the stats for use in > > + * study function to match against the classified packets to choose > > + * the optimal implementation. > > + * On error, returns EINVAL. > > + * On success, returns 0. > > + */ > > +uint32_t mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, > > + struct dpif_miniflow_extract_impl *opt); > > > > #endif /* DPIF_NETDEV_AVX512_EXTRACT */ diff --git > > a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 716e0debf..35c927d55 > > 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -1141,14 +1141,29 @@ dpif_miniflow_extract_impl_set(struct > unixctl_conn *conn, int argc, > > return; > > } > > new_func = opt->extract_func; > > - /* argv[2] is optional datapath instance. If no datapath name is > provided. > > + > > + /* argv[2] is optional packet count, which user can provide along with > > + * study function to set the minimum packet that must be matched in > order > > + * to choose the optimal function. */ > > + uint32_t pkt_cmp_count = 0; > > + uint32_t study_ret; > > + if (argc == 3) { > > + char *err_str; > > + pkt_cmp_count = strtoul(argv[2], &err_str, 10); > > + study_ret = mfex_set_study_pkt_cnt(pkt_cmp_count, opt); > > + } else { > > + /* Default packet compare count when packets count not provided. > */ > > + study_ret = mfex_set_study_pkt_cnt(0, opt); > > + } > > + > > + /* argv[3] is optional datapath instance. If no datapath name is > provided. > > * and only one datapath exists, the one existing datapath is reprobed. > > */ > > ovs_mutex_lock(&dp_netdev_mutex); > > struct dp_netdev *dp = NULL; > > > > - if (argc == 3) { > > - dp = shash_find_data(&dp_netdevs, argv[2]); > > + if (argc == 4) { > > + dp = shash_find_data(&dp_netdevs, argv[3]); > > } else if (shash_count(&dp_netdevs) == 1) { > > dp = shash_first(&dp_netdevs)->data; > > } > > @@ -1182,7 +1197,14 @@ dpif_miniflow_extract_impl_set(struct > > unixctl_conn *conn, int argc, > > > > /* Reply with success to command. */ > > struct ds reply = DS_EMPTY_INITIALIZER; > > - ds_put_format(&reply, "Miniflow implementation set to %s.\n", > mfex_name); > > + if (study_ret == 0) { > > + ds_put_format(&reply, "Miniflow implementation set to %s" > > + "(minimum packet to study: %d)\n", > > + mfex_name, pkt_cmp_count); > > + } else { > > + ds_put_format(&reply, "Miniflow implementation set to %s.\n", > > + mfex_name); > > + } > > const char *reply_str = ds_cstr(&reply); > > VLOG_INFO("%s", reply_str); > > unixctl_command_reply(conn, reply_str); @@ -1416,8 +1438,8 @@ > > dpif_netdev_init(void) > > 1, 2, dpif_netdev_impl_set, > > NULL); > > unixctl_command_register("dpif-netdev/miniflow-parser-set", > > - "miniflow implementation name [dp]", > > - 1, 2, dpif_miniflow_extract_impl_set, > > + "miniflow implementation name [pkt_cnt] [dp]", > > + 1, 3, dpif_miniflow_extract_impl_set, > > Given that this command is generic for all implementations and it is now > accepting arguments, I suggest we use parameters to identify them properly. > For example: [-count pkt_cnt]. > Done. > > NULL); > > unixctl_command_register("dpif-netdev/dpif-get", "", > > 0, 0, dpif_netdev_impl_get, > > -- > > 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
