[ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract

Amber, Kumar kumar.amber at intel.com
Mon Jun 28 15:01:49 UTC 2021


Hi Flavio,

Thanks a lot for the review my replies are inline.

<Snipped>

> > +        uint32_t mfex_hit = (mf_mask & (1 << i));
> 
> This is actually a bool.
> 

Fixed in upcoming v5.
> >
> >          /* Check for partial hardware offload mark. */
> >          uint32_t mark;
> 
> I find 'ovs-appctl dpif-netdev/miniflow-parser-set disable'
> misleading because we are not disabling miniflow parser at all.
> It is just using scalar function instead.
> 

Fixed in upcoming v5. Changed from Disable to scalar.
> 
> > +};
> > +
> > +BUILD_ASSERT_DECL(MFEX_IMPLS_MAX_SIZE > ARRAY_SIZE(mfex_impls));
> > +
> > +int32_t
> > +dpif_miniflow_extract_opt_get(const char *name,
> > +                              struct dpif_miniflow_extract_impl
> > +**opt) {
> > +    ovs_assert(opt);
> > +    ovs_assert(name);
> > +
> > +    uint32_t i;
> > +    for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
> > +        if (strcmp(name, mfex_impls[i].name) == 0) {
> > +            *opt = &mfex_impls[i];
> > +            return 0;
> > +        }
> > +    }
> > +    return -ENOTSUP;
> > +}
> > +
> > +void
> > +dpif_miniflow_extract_init(void)
> > +{
> > +    /* Call probe on each impl, and cache the result. */
> > +    uint32_t i;
> > +    for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
> > +        int avail = 1;
> 
> Can this be a bool instead?

Fixed in upcoming v5.
> 
> > +        if (mfex_impls[i].probe) {
> > +            /* Return zero is success, non-zero means error. */
> > +            avail = (mfex_impls[i].probe() == 0);
> > +        }
> > +        VLOG_INFO("Miniflow Extract implementation %s (available: %s)\n",
> > +                  mfex_impls[i].name, avail ? "True" : "False");
> > +        mfex_impls[i].available = avail;
> > +    }
> > +}
> > +
> > +int32_t
> > +dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl
> > +**out_ptr) {
> > +    if (out_ptr == NULL) {
> > +        return -EINVAL;
> > +    }
> 
> missing space.

Fixed in upcoming v5.
> 
> > +    *out_ptr = mfex_impls;
> > +    return ARRAY_SIZE(mfex_impls);
> > +}
> > diff --git a/lib/dpif-netdev-private-extract.h
> > +struct netdev_flow_key;
> > +
> > +/* Function pointer prototype to be implemented in the optimized
> > +miniflow
> > + * extract code.
> > + * returns the hitmask of the processed packets on success.
> > + * returns zero on failure.
> > + */
> > +typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch
> *batch,
> > +                                          struct netdev_flow_key *keys,
> > +                                          uint32_t keys_size,
> > +                                          odp_port_t in_port,
> > +                                          void *pmd_handle);
> 
> The pmd_handle is void here, but some callers in future patches cast it to
> struct dp_netdev_pmd_thread. After the refactoring, that struct is declared
> in dpif-netdev-private-thread.h, so can we use that instead?
> 
> 
Fixed in upcoming v5. Changed structure than void.
> > +
> > +/* Probe function is used to detect if this CPU has the ISA required
> > + * to run the optimized miniflow implementation.
> > + * returns one on successful probe.
> > + * returns zero on failure.
> > + */
> > +typedef int32_t (*miniflow_extract_probe)(void);
> > +
> > +/* Structure representing the attributes of an optimized
> > +implementation. */ struct dpif_miniflow_extract_impl {
> > +    /* When non-zero, this impl has passed the probe() checks. */
> > +    uint8_t available;
> 
> Can this be of type bool?
> 
Fixed in upcoming v5.
> > +
> > +    /* Probe function is used to detect if this CPU has the ISA required
> > +     * to run the optimized miniflow implementation.
> > +     */
> > +    miniflow_extract_probe probe;
> > +
> > +    /* Function to call to extract miniflows for a burst of packets. */
> > +    miniflow_extract_func extract_func;
> > +
> > +    /* Name of the optimized implementation. */
> > +    char *name;
> > +};
> > +
> > +/* Retrieve the opt structure for the requested implementation by name.
> > + * Returns zero on success, and opt points to a valid struct, or
> > + * returns a negative failure status.
> > + * -ENOTSUP : invalid name requested
> > + */
> > +int32_t
> > +dpif_miniflow_extract_opt_get(const char *name,
> > +                              struct dpif_miniflow_extract_impl
> > +**opt);
> > +
> > +/* Initializes the available miniflow extract implementations by
> > +probing for
> > + * the CPU ISA requirements. As the runtime available CPU ISA does
> > +not change
> > + * and the required ISA of the implementation also does not change,
> > +it is safe
> > + * to cache the probe() results, and not call probe() at runtime.
> > + */
> > +void
> > +dpif_miniflow_extract_init(void);
> > +
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > f316835a4..567ebd952 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -46,6 +46,7 @@
> >  #include "dpif.h"
> >  #include "dpif-netdev-lookup.h"
> >  #include "dpif-netdev-perf.h"
> > +#include "dpif-netdev-private-extract.h"
> >  #include "dpif-provider.h"
> >  #include "dummy.h"
> >  #include "fat-rwlock.h"
> > @@ -1089,6 +1090,102 @@ dpif_netdev_impl_set(struct unixctl_conn
> *conn, int argc,
> >      ds_destroy(&reply);
> >  }
> >
> > +static void
> > +dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> > +                     const char *argv[] OVS_UNUSED, void *aux
> > +OVS_UNUSED) {
> > +    struct dpif_miniflow_extract_impl *mfex_impls;
> > +    uint32_t count = dpif_miniflow_extract_info_get(&mfex_impls);
> > +    if (count == 0) {
> > +        unixctl_command_reply_error(conn, "error getting mfex names");
> > +        return;
> > +    }
> > +
> > +    /* Add all mfex functions to reply string. */
> > +    struct ds reply = DS_EMPTY_INITIALIZER;
> > +    ds_put_cstr(&reply, "Available Optimized Miniflow Extracts:\n");
> > +    for (uint32_t i = 0; i < count; i++) {
> > +        ds_put_format(&reply, "  %s (available: %s)\n",
> > +                      mfex_impls[i].name, mfex_impls[i].available ?
> > +                      "True" : "False");
> > +    }
> 
> Shouldn't this be inside private-extract.c ?
> See the comments for the DPIF dp_netdev_impl_get command:
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384341.html
> 
> 
Fixed in v5.
> 
> > +    unixctl_command_reply(conn, ds_cstr(&reply));
> > +    ds_destroy(&reply);
> > +}
> > +
> > +static void
> > +dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
> > +                     const char *argv[], void *aux OVS_UNUSED) {
> > +    /* This function requires just one parameter, the miniflow name.
> > +     * A second optional parameter can identify the datapath instance.
> > +     */
> > +    const char *mfex_name = argv[1];
> > +
> > +    static const char *error_description[2] = {
> > +        "Unknown miniflow implementation",
> > +        "implementation doesn't exist",
> > +    };
> > +    struct dpif_miniflow_extract_impl *opt;
> > +    miniflow_extract_func new_func;
> > +    int32_t err = dpif_miniflow_extract_opt_get(mfex_name, &opt);
> > +    if (err) {
> > +        struct ds reply = DS_EMPTY_INITIALIZER;
> > +        ds_put_format(&reply,
> > +                    "Miniflow implementation not available: %s %s.\n",
> > +                    error_description[ (err == -ENOTSUP) ], mfex_name);
> > +        const char *reply_str = ds_cstr(&reply);
> > +        unixctl_command_reply(conn, reply_str);
> > +        VLOG_INFO("%s", reply_str);
> > +        ds_destroy(&reply);
> > +        return;
> > +    }
> > +    new_func = opt->extract_func;
> > +    /* argv[2] 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]);
> > +    } else if (shash_count(&dp_netdevs) == 1) {
> > +        dp = shash_first(&dp_netdevs)->data;
> > +    }
> > +
> > +    if (!dp) {
> > +        ovs_mutex_unlock(&dp_netdev_mutex);
> > +        unixctl_command_reply_error(conn,
> > +                                    "please specify an existing datapath");
> > +        return;
> > +    }
> > +
> > +    /* Get PMD threads list. */
> > +    size_t n;
> > +    struct dp_netdev_pmd_thread **pmd_list;
> > +    sorted_poll_thread_list(dp, &pmd_list, &n);
> > +
> > +    for (size_t i = 0; i < n; i++) {
> > +        struct dp_netdev_pmd_thread *pmd = pmd_list[i];
> > +        if (pmd->core_id == NON_PMD_CORE_ID) {
> > +            continue;
> > +        }
> > +
> > +        /* Set PMD threads miniflow implementation to requested one. */
> > +        pmd->miniflow_extract_opt = *new_func;
> > +    };
> > +
> > +    ovs_mutex_unlock(&dp_netdev_mutex);
> > +
> > +    /* Reply with success to command. */
> > +    struct ds reply = DS_EMPTY_INITIALIZER;
> > +    ds_put_format(&reply, "Miniflow implementation set to %s.\n",
> mfex_name);
> > +    const char *reply_str = ds_cstr(&reply);
> > +    unixctl_command_reply(conn, reply_str);
> > +    VLOG_INFO("%s", reply_str);
> > +    ds_destroy(&reply);
> 
> This is very similar to the dpif_netdev_impl_set() reviewed in
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384336.html
> 
> In this case you still need to provide dpif_miniflow_extract_opt_get(),
> however the callers seem to be all in -*-extract*.c
> 
> 

Fixed in v5.
> > +}
> > +
> >  static void
> >  dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
> >                            const char *argv[], void *aux OVS_UNUSED)
> > @@ -1315,9 +1412,16 @@ dpif_netdev_init(void)
> >                               "dpif_implementation_name [dp]",
> >                               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,
> > +                             NULL);
> >      unixctl_command_register("dpif-netdev/dpif-get", "",
> >                               0, 0, dpif_netdev_impl_get,
> >                               NULL);
> > +    unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
> > +                             0, 0, dpif_miniflow_extract_impl_get,
> > +                             NULL);
> 
> Already mentioned in another review, but please don't separate get and set
> functions.
> 

Fixed in upcoming v5.
> >      return 0;
> >  }
> >
> > @@ -1461,6 +1565,8 @@ create_dp_netdev(const char *name, const
> struct
> > dpif_class *class,
> >
> >      dp->conntrack = conntrack_init();
> >
> > +    dpif_miniflow_extract_init();
> > +
> 
> This is using a different logic than DPIF. The DPIF calls probe when the
> implementation is requested. This one seems better assuming that calling
> .probe() are harmless.
> 

Sure .
> 
> >      atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN);
> >      atomic_init(&dp->tx_flush_interval, DEFAULT_TX_FLUSH_INTERVAL);
> >
> > @@ -6176,6 +6282,9 @@ dp_netdev_configure_pmd(struct
> dp_netdev_pmd_thread *pmd, struct dp_netdev *dp,
> >      /* Initialize DPIF function pointer to the default configured version. */
> >      pmd->netdev_input_func = dp_netdev_impl_get_default();
> >
> > +    /*Init default miniflow_extract function */
> > +    pmd->miniflow_extract_opt = NULL;
> > +
> 
> That should come from dpif_miniflow_extract_get_default().
> I see that you are actually adding that on one later patch, but can we have a
> function returning NULL for now? And the comment needs fixing.

Fixed in v5.
> 
> >      /* init the 'flow_cache' since there is no
> >       * actual thread created for NON_PMD_CORE_ID. */
> >      if (core_id == NON_PMD_CORE_ID) { @@ -6730,10 +6839,12 @@
> > dfc_processing(struct dp_netdev_pmd_thread *pmd,  {
> >      struct netdev_flow_key *key = &keys[0];
> >      size_t n_missed = 0, n_emc_hit = 0, n_phwol_hit = 0;
> > +    struct dp_packet_batch single_packet;
> >      struct dfc_cache *cache = &pmd->flow_cache;
> >      struct dp_packet *packet;
> >      const size_t cnt = dp_packet_batch_size(packets_);
> >      uint32_t cur_min = pmd->ctx.emc_insert_min;
> > +    int mf_ret;
> >      int i;
> >      uint16_t tcp_flags;
> >      bool smc_enable_db;
> > @@ -6786,8 +6897,19 @@ dfc_processing(struct dp_netdev_pmd_thread
> *pmd,
> >                  continue;
> >              }
> >          }
> > -
> > -        miniflow_extract(packet, &key->mf);
> > +        /* Set the count and packet for miniflow_opt with batch_size 1. */
> > +        if ((pmd->miniflow_extract_opt) && (!md_is_valid)) {
> 
> The single_packet and mf_ret are not in reverse xmas tree order, and they
> are not needed outside of this scope. My suggestion to improve readability
> and fix the coding style issue is to declare both at this point. Does that make
> sense?
> 
> 

Fixed in upcoming v5.
> > +            single_packet.count = 1;
> > +            single_packet.packets[0] = packet;
> > +            mf_ret = pmd->miniflow_extract_opt(&single_packet, key, 1,
> > +                                               port_no, (void *) pmd);
> > +            /* Fallback to original miniflow_extract if there is a miss. */
> > +            if (!mf_ret) {
> > +                miniflow_extract(packet, &key->mf);
> > +            }
> > +        } else {
> > +            miniflow_extract(packet, &key->mf);
> > +        }
> >          key->len = 0; /* Not computed yet. */
> >          key->hash =
> >                  (md_is_valid == false)
> > --
> > 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