[ovs-dev] [v4 06/12] dpif-netdev: Add additional packet count parameter for study function
Amber, Kumar
kumar.amber at intel.com
Tue Jun 29 04:14:45 UTC 2021
Hi Flavio,
Thanks Again, replies are inline.
> -----Original Message-----
> From: Flavio Leitner <fbl at sysclose.org>
> Sent: Monday, June 28, 2021 8:26 AM
> To: Amber, Kumar <kumar.amber at intel.com>
> Cc: dev at openvswitch.org; i.maximets at ovn.org
> 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 <kumar.amber at intel.com>
> > ---
> > 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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
More information about the dev
mailing list