Hi Eelco, Thanks for your comments. My response is inline.
Cian > -----Original Message----- > From: Eelco Chaudron <[email protected]> > Sent: Friday 25 June 2021 16:14 > To: Ferriter, Cian <[email protected]> > Cc: Flavio Leitner <[email protected]>; [email protected]; > [email protected] > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
