[ovs-dev] [PATCH v3 2/7] dpif-netdev: add subtable lookup set command

William Tu u9012063 at gmail.com
Tue Jun 16 15:41:48 UTC 2020


Thanks for the patch. I have some minor comments.

On Wed, Jun 10, 2020 at 3:47 AM Harry van Haaren
<harry.van.haaren at intel.com> wrote:
the commit title should add "lookup prio set command"?

>
> This commit adds a command for the dpif-netdev to set a specific
> lookup function to a particular priority level. The command enables
> runtime switching of the dpcls subtable lookup implementation.
>
> Selection is performed based on a priority. Higher priorities take
> precedence, eg; priotity 5 will be selected instead of a priority 3.
>
> The two options available are 'autovalidator' and 'generic'.
> The below command will set a new priority for the given function:
> $ ovs-appctl dpif-netdev/subtable-lookup-set generic 2
add prio

>
> The autovalidator implementation can be selected at runtime now:
> $ ovs-appctl dpif-netdev/subtable-lookup-set autovalidator 5
add prio
>
> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
>
> ---
>
> v3
> - Add automatic reprobe after changing priorities
> --- Refactored from previous 1-second timeout based reprobe WIP-hack
> - Add VLOG entries for changed dpcls and subtable counts
> --- Also return the updated counts to the issuing command for visibility
> - Clarify command by adding "prio" to the name
> --- New command name is "dpif-netdev/subtable-lookup-prio-set"
> --- Please note this new command change - previous command is now invalid
> ---
>  lib/dpif-netdev.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 5e101e054..30806af16 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -258,6 +258,7 @@ struct dp_packet_flow_map {
>  static void dpcls_init(struct dpcls *);
>  static void dpcls_destroy(struct dpcls *);
>  static void dpcls_sort_subtable_vector(struct dpcls *);
> +static uint32_t dpcls_subtable_lookup_reprobe(struct dpcls *cls);
>  static void dpcls_insert(struct dpcls *, struct dpcls_rule *,
>                           const struct netdev_flow_key *mask);
>  static void dpcls_remove(struct dpcls *, struct dpcls_rule *);
> @@ -860,6 +861,9 @@ dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
>                                 bool purge);
>  static int dpif_netdev_xps_get_tx_qid(const struct dp_netdev_pmd_thread *pmd,
>                                        struct tx_port *tx);
> +static inline struct dpcls *
> +dp_netdev_pmd_lookup_dpcls(struct dp_netdev_pmd_thread *pmd,
> +                           odp_port_t in_port);
>
>  static inline bool emc_entry_alive(struct emc_entry *ce);
>  static void emc_clear_entry(struct emc_entry *ce);
> @@ -1260,6 +1264,97 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>      *n = k;
>  }
>
> +static void
> +dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc,
> +                                const char *argv[], void *aux OVS_UNUSED)
> +{
> +    /* This function requires 2 parameters (argv[1] and argv[2]) to execute.
> +     *   argv[1] is subtable name
> +     *   argv[2] is priority
> +     *   argv[3] is the datapath name (optional if only 1 datapath exists)
> +     */
> +    const char *func_name = argv[1];
> +
> +    errno = 0;
> +    char *err_char;
> +    uint32_t new_prio = strtoul(argv[2], &err_char, 10);
> +    if (errno != 0 || new_prio > UINT8_MAX) {
> +        unixctl_command_reply_error(conn,
> +            "error converting priority, use integer in range 0-255\n");
> +        return;
> +    }
> +
> +    int32_t err = dpcls_subtable_set_prio(func_name, new_prio);
> +    if (err) {
> +        unixctl_command_reply_error(conn,
> +            "error, subtable lookup function not found\n");
> +        return;
> +    }
> +
> +    /* 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 == 4) {
> +        dp = shash_find_data(&dp_netdevs, argv[3]);
> +    } 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, required to get DPCLS instances */
> +    size_t n;
> +    uint32_t lookup_dpcls_changed = 0;
> +    uint32_t lookup_subtable_changed = 0;
> +    struct dp_netdev_pmd_thread **pmd_list;
> +    sorted_poll_thread_list(dp, &pmd_list, &n);
> +
> +    /* take port mutex as HMAP iters over them */
> +    ovs_mutex_lock(&dp->port_mutex);
> +
> +    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;
> +        }
> +
> +        struct dp_netdev_port *port = NULL;
> +        HMAP_FOR_EACH (port, node, &dp->ports) {
> +            odp_port_t in_port = port->port_no;
> +            struct dpcls *cls = dp_netdev_pmd_lookup_dpcls(pmd, in_port);
> +            if (!cls) {
> +                continue;
> +            }
> +            uint32_t subtbl_changes = dpcls_subtable_lookup_reprobe(cls);
> +            if (subtbl_changes) {
> +                lookup_dpcls_changed++;
> +                lookup_subtable_changed += subtbl_changes;
> +            }
> +        }
> +    }
> +
> +    /* release port mutex before netdev mutex */
> +    ovs_mutex_unlock(&dp->port_mutex);
> +    ovs_mutex_unlock(&dp_netdev_mutex);
> +
> +    struct ds reply = DS_EMPTY_INITIALIZER;
> +    ds_put_format(&reply,
> +        "Lookup priority change affected %d dpcls ports and %d subtables.\n",
> +        lookup_dpcls_changed, lookup_subtable_changed);
> +    const char *reply_str = ds_cstr(&reply);
> +    unixctl_command_reply(conn, reply_str);
> +    VLOG_INFO("%s", reply_str);
> +    ds_destroy(&reply);
> +}
> +
>  static void
>  dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
>                            const char *argv[], void *aux OVS_UNUSED)
> @@ -1429,6 +1524,10 @@ dpif_netdev_init(void)
>                               "[-us usec] [-q qlen]",
>                               0, 10, pmd_perf_log_set_cmd,
>                               NULL);
> +    unixctl_command_register("dpif-netdev/subtable-lookup-prio-set",
> +                            "[lookup_func] [prio] [dp]",
alignment, add one more space
Thanks

William


More information about the dev mailing list