[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