[ovs-dev] [PATCH 2/2] dpif-netdev: Allow direct destroy of 'struct dp_netdev_port'.
Pravin Shelar
pshelar at nicira.com
Wed Nov 12 23:03:42 UTC 2014
On Fri, Nov 7, 2014 at 2:51 PM, Alex Wang <alexw at nicira.com> wrote:
> Before this commit, when 'struct dp_netdev_port' is deleted from
> 'dpif-netdev' datapath, if there is pmd thread, the pmd thread
> will release the last reference to the port and ovs-rcu postpone
> the destroy. However, the delayed close of object like 'struct
> netdev' could cause failure in immediate re-add or reconfigure of
> the same device.
>
> To fix the above issue, this commit uses condition variable and
> makes the main thread wait for pmd thread to release the reference
> when deleting port. Then, the main thread can directly destroy the
> port.
>
> Reported-by: Cian Ferriter <cian.ferriter at intel.com>
> Signed-off-by: Alex Wang <alexw at nicira.com>
Thanks for fixing this.
> ---
> lib/dpif-netdev.c | 61 ++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index bee9330..ad2febc 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -370,6 +370,10 @@ static void dp_netdev_actions_free(struct dp_netdev_actions *);
> struct dp_netdev_pmd_thread {
> struct dp_netdev *dp;
> struct cmap_node node; /* In 'dp->poll_threads'. */
> +
> + pthread_cond_t cond; /* For synchronizing pmd thread reload. */
> + struct ovs_mutex cond_mutex; /* Mutex for condition variable. */
> +
> /* Per thread exact-match cache. Note, the instance for cpu core
> * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
> * need to be protected (e.g. by 'dp_netdev_mutex'). All other
> @@ -416,6 +420,7 @@ static void dp_netdev_input(struct dp_netdev_pmd_thread *,
> struct dpif_packet **, int cnt);
>
> static void dp_netdev_disable_upcall(struct dp_netdev *);
> +void dp_netdev_pmd_signal_cond(struct dp_netdev_pmd_thread *pmd);
> static void dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd,
> struct dp_netdev *dp, int index,
> int core_id, int numa_id);
> @@ -761,7 +766,14 @@ dp_netdev_reload_pmd__(struct dp_netdev_pmd_thread *pmd)
> {
> int old_seq;
>
> + if (pmd->core_id == NON_PMD_CORE_ID) {
> + return;
> + }
> +
> + ovs_mutex_lock(&pmd->cond_mutex);
> atomic_add_relaxed(&pmd->change_seq, 1, &old_seq);
> + ovs_mutex_cond_wait(&pmd->cond, &pmd->cond_mutex);
> + ovs_mutex_unlock(&pmd->cond_mutex);
> }
>
> /* Causes all pmd threads to reload its tx/rx devices.
> @@ -972,27 +984,21 @@ port_try_ref(struct dp_netdev_port *port)
> }
>
> static void
> -port_destroy__(struct dp_netdev_port *port)
> -{
> - int n_rxq = netdev_n_rxq(port->netdev);
> - int i;
> -
> - netdev_close(port->netdev);
> - netdev_restore_flags(port->sf);
> -
> - for (i = 0; i < n_rxq; i++) {
> - netdev_rxq_close(port->rxq[i]);
> - }
> - free(port->rxq);
> - free(port->type);
> - free(port);
> -}
> -
> -static void
> port_unref(struct dp_netdev_port *port)
> {
> if (port && ovs_refcount_unref_relaxed(&port->ref_cnt) == 1) {
> - ovsrcu_postpone(port_destroy__, port);
> + int n_rxq = netdev_n_rxq(port->netdev);
> + int i;
> +
> + netdev_close(port->netdev);
> + netdev_restore_flags(port->sf);
> +
> + for (i = 0; i < n_rxq; i++) {
> + netdev_rxq_close(port->rxq[i]);
> + }
> + free(port->rxq);
> + free(port->type);
> + free(port);
> }
> }
>
> @@ -2296,6 +2302,10 @@ reload:
> emc_cache_init(&pmd->flow_cache);
> poll_cnt = pmd_load_queues(pmd, &poll_list, poll_cnt);
>
> + /* Signal here to make sure the pmd finishes
> + * reloading the updated configuration. */
> + dp_netdev_pmd_signal_cond(pmd);
> +
I think dp_netdev_pmd_reload_done() is better name.
> for (;;) {
> int i;
>
> @@ -2317,7 +2327,6 @@ reload:
> }
> }
> }
> -
> emc_cache_uninit(&pmd->flow_cache);
>
> if (!latch_is_set(&pmd->exit_latch)){
> @@ -2328,6 +2337,8 @@ reload:
> port_unref(poll_list[i].port);
> }
>
> + dp_netdev_pmd_signal_cond(pmd);
> +
> free(poll_list);
> return NULL;
> }
> @@ -2362,6 +2373,14 @@ dpif_netdev_enable_upcall(struct dpif *dpif)
> dp_netdev_enable_upcall(dp);
> }
>
> +void
> +dp_netdev_pmd_signal_cond(struct dp_netdev_pmd_thread *pmd)
> +{
> + ovs_mutex_lock(&pmd->cond_mutex);
> + xpthread_cond_signal(&pmd->cond);
> + ovs_mutex_unlock(&pmd->cond_mutex);
> +}
> +
> /* Returns the pointer to the dp_netdev_pmd_thread for non-pmd threads. */
> static struct dp_netdev_pmd_thread *
> dp_netdev_get_nonpmd(struct dp_netdev *dp)
> @@ -2398,6 +2417,8 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp,
> pmd->numa_id = numa_id;
> latch_init(&pmd->exit_latch);
> atomic_init(&pmd->change_seq, PMD_INITIAL_SEQ);
> + xpthread_cond_init(&pmd->cond, NULL);
> + ovs_mutex_init(&pmd->cond_mutex);
> /* init the 'flow_cache' since there is no
> * actual thread created for NON_PMD_CORE_ID. */
> if (core_id == NON_PMD_CORE_ID) {
> @@ -2424,6 +2445,8 @@ dp_netdev_del_pmd(struct dp_netdev_pmd_thread *pmd)
> }
> cmap_remove(&pmd->dp->poll_threads, &pmd->node, hash_int(pmd->core_id, 0));
> latch_destroy(&pmd->exit_latch);
> + xpthread_cond_destroy(&pmd->cond);
> + ovs_mutex_destroy(&pmd->cond_mutex);
> free(pmd);
> }
>
Acked-by: Pravin B Shelar <pshelar at nicira.com>
> --
> 1.7.9.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list