[ovs-dev] [PATCH v6 2/6] dpif-netdev: add subtable lookup prio set command.

Stokes, Ian ian.stokes at intel.com
Thu Jul 9 17:19:22 UTC 2020



On 7/2/2020 6:42 PM, Harry van Haaren wrote:
> 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.
Minor typo above 'priority'
> If lookup functions have the same priority, the first one in the list
> is selected.
> 
> 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-prio-set generic 2
> 
> The autovalidator implementation can be selected at runtime now:
> $ ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 5
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> Acked-by: William Tu <u9012063 at gmail.com>
> 

LGTM, tested OK, few minor typos and style comments that can be fixed 
upon commit. Few comments below.

> ---
> 
> v5:
> - Add Ack from mailing list
> - Rebase to latest master (fixing command adding conflict)
> 
> v4:
> - Align prio-set command in code (William Tu)
> - Improve commit title & update commit message (William Tu)
> 
> 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 0468064c9..758323a0e 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -259,6 +259,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 *);
> @@ -891,6 +892,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);
> @@ -1291,6 +1295,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;
> +    }

So in testing above I spotted that you are able to set the priority of a 
subtable lookup function even though no datapath or subtables exist for 
that lookup function to operate on. Is this expected?

I understand that the same lookup functions can have the different 
priorities on different bridges, just thinking in the case that no 
datapath exists should you be unable to set the priority?

BR
Ian
> +
> +    /* 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)
> @@ -1506,6 +1601,10 @@ dpif_netdev_init(void)
>       unixctl_command_register("dpif-netdev/bond-show", "[dp]",
>                                0, 1, dpif_netdev_bond_show,
>                                NULL);
> +    unixctl_command_register("dpif-netdev/subtable-lookup-prio-set",
> +                             "[lookup_func] [prio] [dp]",
> +                             2, 3, dpif_netdev_subtable_lookup_set,
> +                             NULL);
>       return 0;
>   }
>   
> @@ -8426,6 +8525,28 @@ dpcls_find_subtable(struct dpcls *cls, const struct netdev_flow_key *mask)
>       return dpcls_create_subtable(cls, mask);
>   }
>   
> +/* Checks for the best available implementation for each subtable lookup
> + * function, and assigns it as the lookup function pointer for each subtable.
> + * Returns the number of subtables that have changed lookup implementation.
> + */
> +static uint32_t
> +dpcls_subtable_lookup_reprobe(struct dpcls *cls)
> +{
> +    struct pvector *pvec = &cls->subtables;
> +    uint32_t subtables_changed = 0;
> +    struct dpcls_subtable *subtable = NULL;
> +
> +    PVECTOR_FOR_EACH (subtable, pvec) {
> +        uint32_t u0_bits = subtable->mf_bits_set_unit0;
> +        uint32_t u1_bits = subtable->mf_bits_set_unit1;
> +        void *old_func = subtable->lookup_func;
> +        subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits, u1_bits);
> +        subtables_changed += (old_func != subtable->lookup_func);
> +    }
> +    pvector_publish(pvec);
> +
> +    return subtables_changed;
> +}
>   
>   /* Periodically sort the dpcls subtable vectors according to hit counts */
>   static void
> 


More information about the dev mailing list