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 <[email protected]>
> Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.")
> Cc: [email protected]
> ---
> 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/[email protected]/
>
> 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,
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
