[ovs-dev] [v4 06/12] dpif-netdev: Add additional packet count parameter for study function
Amber, Kumar
kumar.amber at intel.com
Fri Jun 25 02:47:49 UTC 2021
Hi Ian ,
Many Thanks Again for reviews , my replies are inline.
> > 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.
>
> I'd reword above as follows for typos and grammar.
>
> "A user can also provide an additional packet count parameter which is the
> minimum number of packets that OVS must study before choosing an
> optimal implementation. If no packet count is provided then the default
> value is chosen."
>
Rephrase taken into v5.
> > +
> > +Study can be selected with packet count by the following command ::
> > +
> > + $ ovs-appctl dpif-netdev/miniflow-parser-set study 1024
> >
> > 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;
>
> Would prefer variable declarations outside of functions to be declared at the
> beginning of the file after imports and defines.
>
Fixed 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;
> > +}
> > +
> > 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);
>
> Would add white space here to split comment below from function above.
>
Fixed in v5.
>
> BR
> Ian
> > +/* 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,
> > NULL);
> > unixctl_command_register("dpif-netdev/dpif-get", "",
> > 0, 0, dpif_netdev_impl_get,
> > --
> > 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