[ovs-dev] [v13 05/12] dpif-netdev: Add command to switch dpif implementation.

Ferriter, Cian cian.ferriter at intel.com
Tue Jun 29 08:17:06 UTC 2021


Hi Eelco,

Thanks for your comments. My response is inline.

Cian

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Friday 25 June 2021 16:14
> To: Ferriter, Cian <cian.ferriter at intel.com>
> Cc: Flavio Leitner <fbl at sysclose.org>; ovs-dev at openvswitch.org; i.maximets at ovn.org
> Subject: Re: [ovs-dev] [v13 05/12] dpif-netdev: Add command to switch dpif implementation.
> 
> 
> 
> On 24 Jun 2021, at 13:53, Ferriter, Cian wrote:
> 
> > Hi Flavio,
> >
> > Thanks for the review. My responses are inline.
> >
> > Cian
> >
> 
> <SNIP>
> 
> >>>
> >>> +static void
> >>> +dpif_netdev_impl_set(struct unixctl_conn *conn, int argc,
> >>> +                     const char *argv[], void *aux OVS_UNUSED)
> >>> +{
> >>> +    /* This function requires just one parameter, the DPIF name.
> >>> +     * A second optional parameter can identify the datapath instance.
> >>> +     */
> >>> +    const char *dpif_name = argv[1];
> >>> +
> >>> +    static const char *error_description[2] = {
> >>> +        "Unknown DPIF implementation",
> >>> +        "CPU doesn't support the required instruction for",
> >>> +    };
> >>> +
> >>> +    dp_netdev_input_func new_func;
> >>> +    int32_t err = dp_netdev_impl_get_by_name(dpif_name, &new_func);
> >>> +    if (err) {
> >>> +        struct ds reply = DS_EMPTY_INITIALIZER;
> >>> +        ds_put_format(&reply, "DPIF implementation not available: %s %s.\n",
> >>> +                      error_description[ (err == -ENOTSUP) ], dpif_name);
> >>> +        const char *reply_str = ds_cstr(&reply);
> >>> +        unixctl_command_reply(conn, reply_str);
> >>> +        VLOG_INFO("%s", reply_str);
> >>> +        ds_destroy(&reply);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    /* argv[2] 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 == 3) {
> >>> +        dp = shash_find_data(&dp_netdevs, argv[2]);
> >>> +    } 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. */
> >>> +    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;
> >>> +        }
> >>> +
> >>> +        /* Set PMD threads DPIF implementation to requested one. */
> >>> +        pmd->netdev_input_func = *new_func;
> >>> +    };
> >>> +
> >>> +    ovs_mutex_unlock(&dp_netdev_mutex);
> >>> +
> >>> +    /* Set the default implementation for PMD threads created in the future. */
> >>> +    dp_netdev_impl_set_default(*new_func);
> >>
> >> I checked other patches and it seems this interface could be simplified
> >> and would fix the set_default() above to be more robust.
> >> What do you think of:
> >>
> >>    1) lock dp_netdev_mutex
> >>    2) Check if the DP argument is valid.
> >>    3) Use a new dp_netdev_impl_set_default_by_name()
> >>       That fails if the name is not available or set the default input
> >>       hiding the function pointer from outside.
> >>    4) Loop on each pmd doing the same as in dp_netdev_configure_pmd():
> >>       pmd->netdev_input_func = dp_netdev_impl_get_default();
> >>    5) unlock dp_netdev_mutex
> >>
> >> It will hold the lock a bit more time but we don't expect to have
> >> several inputs and no frequent input changes, so we should be fine.
> >>
> >
> > Good idea. Hiding the function pointer from here is a nice improvement. I'll rework it to do this.
> 
> 
> Do we also assume that setting the function pointer happens atomically on all supported architectures?
> I would assume this requires an atomic set?
> 

This is a good point. We want an atomic set here. I will push the below fix with the next version of the patchset.

-            pmd->netdev_input_func = dp_netdev_impl_get_default();
+            dp_netdev_input_func default_func = dp_netdev_impl_get_default();
+            atomic_uintptr_t *pmd_func = (void *)&pmd->netdev_input_func;
+            atomic_store_relaxed(pmd_func, (uintptr_t)default_func);

> //Eelco



More information about the dev mailing list