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

Eelco Chaudron echaudro at redhat.com
Fri Jun 25 15:14:25 UTC 2021



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?

//Eelco



More information about the dev mailing list