[ovs-dev] [PATCH] dpif-netdev: apply subtable-lookup-prio-set on any datapath

Ilya Maximets i.maximets at ovn.org
Wed Jun 23 19:02:12 UTC 2021


On 6/23/21 8:54 PM, Timothy Redaelli wrote:
> Currently, if you try to set subtable-lookup-prio-set when you don't have
> any datapath (for example if an user wants to set AVX512 before creating
> any bridge) it sets it globally (dpcls_subtable_set_prio),
> but it returns an error:
> 
>   please specify an existing datapath
>   ovs-appctl: ovs-vswitchd: server returned an error
> 
> and, in this case, the exit code of ovs-appctl is 2.
> 
> This commit changes the behaviour by removing the [dp] optional
> parameter of subtable-lookup-prio-set and by changing the priority
> level on any datapath and globally. This means if you don't have any
> datapath or if you have only one datapath, the behaviour is the same as
> now, but without the confusing error when you don't have any datapath.
> 
> Signed-off-by: Timothy Redaelli <tredaelli at redhat.com>
> Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.")
> Cc: harry.van.haaren at intel.com
> ---
>  lib/dpif-netdev.c | 78 +++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 46 deletions(-)
> ---
> There is no need to change the documentation, since the manpage is not
> merged and it's currently part of another series [1], but the optional
> datapath parameter is not present in the patchset and so there is not need
> to change it. Currently the only document that contains a reference to
> subtable-lookup-prio-set (Documentation/topics/dpdk/bridge.rst),
> doesn't contain any reference to the optional datapath parameter too.
> 
> [1] https://patchwork.ozlabs.org/project/openvswitch/patch/20210617161825.94741-9-cian.ferriter@intel.com/
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 8fa7eb6d4..478eb7cf3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1345,13 +1345,15 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc,
>      /* 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);
> +    uint32_t lookup_dpcls_changed = 0;
> +    uint32_t lookup_subtable_changed = 0;
> +    struct shash_node *node;
>      if (errno != 0 || new_prio > UINT8_MAX) {
>          unixctl_command_reply_error(conn,
>              "error converting priority, use integer in range 0-255\n");
> @@ -1365,58 +1367,42 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc,
>          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);
> +    SHASH_FOR_EACH(node, &dp_netdevs) {
> +        struct dp_netdev *dp = node->data;
>  
> -    /* take port mutex as HMAP iters over them. */
> -    ovs_mutex_lock(&dp->port_mutex);
> +        /* Get PMD threads list, required to get DPCLS instances. */
> +        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;
> -        }
> +        /* take port mutex as HMAP iters over them. */
> +        ovs_mutex_lock(&dp->port_mutex);
>  
> -        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) {
> +        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;
>              }
> -            uint32_t subtbl_changes = dpcls_subtable_lookup_reprobe(cls);
> -            if (subtbl_changes) {
> -                lookup_dpcls_changed++;
> -                lookup_subtable_changed += subtbl_changes;
> +
> +            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);
> +        /* release port mutex before netdev mutex. */
> +        ovs_mutex_unlock(&dp->port_mutex);
> +    }
>      ovs_mutex_unlock(&dp_netdev_mutex);
>  
>      struct ds reply = DS_EMPTY_INITIALIZER;
> @@ -1645,8 +1631,8 @@ dpif_netdev_init(void)
>                               0, 1, dpif_netdev_bond_show,
>                               NULL);
>      unixctl_command_register("dpif-netdev/subtable-lookup-prio-set",
> -                             "[lookup_func] [prio] [dp]",

I'm also wondering why these arguments are in brackets if they are
not optional?

Not for this patch, but in general.

> -                             2, 3, dpif_netdev_subtable_lookup_set,
> +                             "[lookup_func] [prio]",
> +                             2, 2, dpif_netdev_subtable_lookup_set,
>                               NULL);
>      unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", "",
>                               0, 0, dpif_netdev_subtable_lookup_get,
> 



More information about the dev mailing list