Message ID | a3b2546b8bec965212f83ceb7580453f0521fd3e.1624474101.git.tredaelli@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] dpif-netdev: apply subtable-lookup-prio-set on any datapath | expand |
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@redhat.com> > Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.") > Cc: harry.van.haaren@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, >
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]", - 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,
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@redhat.com> Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.") Cc: harry.van.haaren@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/