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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
