Re: [ovs-dev] [PATCH v8 10/16] dpif-netdev: Use hmap for ports.
On 20.04.2016 01:28, diproiettod at vmware.com (Daniele Di Proietto) wrote: > netdev objects are hard to use with RCU, because it's not possible to > split removal and reclamation. Postponing the removal means that the > port is not removed and cannot be readded immediately. Waiting for > reclamation means introducing a quiescent state, and that may introduce > subtle bugs, due to the RCU model we use in userspace. > > This commit changes the port container from cmap to hmap. 'port_mutex' > must be held by readers and writers. This shouldn't have performace > impact, as readers in the fast path use a thread local cache. > > Signed-off-by: Daniele Di Proietto > --- > lib/dpif-netdev.c | 96 > +-- > 1 file changed, 57 insertions(+), 39 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index bd2249e..8cc37e2 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -195,9 +195,10 @@ struct dp_netdev { > > /* Ports. > * > - * Protected by RCU. Take the mutex to add or remove ports. */ > + * Any lookup into 'ports' or any access to the dp_netdev_ports found > + * through 'ports' requires taking 'port_mutex'. */ > struct ovs_mutex port_mutex; > -struct cmap ports; > +struct hmap ports; > struct seq *port_seq; /* Incremented whenever a port changes. */ > > /* Protects access to ofproto-dpif-upcall interface during revalidator > @@ -228,7 +229,8 @@ struct dp_netdev { > }; > > static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev > *dp, > -odp_port_t); > +odp_port_t) > +OVS_REQUIRES(&dp->port_mutex); OVS_REQUIRES(dp->port_mutex); here and 2 times more below. > > enum dp_stat_type { > DP_STAT_EXACT_HIT, /* Packets that had an exact match (emc). */ > @@ -248,7 +250,7 @@ enum pmd_cycles_counter_type { > struct dp_netdev_port { > odp_port_t port_no; > struct netdev *netdev; > -struct cmap_node node; /* Node in dp_netdev's 'ports'. */ > +struct hmap_node node; /* Node in dp_netdev's 'ports'. */ > struct netdev_saved_flags *sf; > unsigned n_rxq; /* Number of elements in 'rxq' */ > struct netdev_rxq **rxq; > @@ -476,9 +478,11 @@ struct dpif_netdev { > }; > > static int get_port_by_number(struct dp_netdev *dp, odp_port_t port_no, > - struct dp_netdev_port **portp); > + struct dp_netdev_port **portp) > +OVS_REQUIRES(dp->port_mutex); > static int get_port_by_name(struct dp_netdev *dp, const char *devname, > -struct dp_netdev_port **portp); > +struct dp_netdev_port **portp) > +OVS_REQUIRES(dp->port_mutex); > static void dp_netdev_free(struct dp_netdev *) > OVS_REQUIRES(dp_netdev_mutex); > static int do_add_port(struct dp_netdev *dp, const char *devname, > @@ -522,7 +526,8 @@ dp_netdev_add_rxq_to_pmd(struct dp_netdev_pmd_thread *pmd, > struct dp_netdev_port *port, struct netdev_rxq *rx); > static struct dp_netdev_pmd_thread * > dp_netdev_less_loaded_pmd_on_numa(struct dp_netdev *dp, int numa_id); > -static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp); > +static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp) > +OVS_REQUIRES(dp->port_mutex); > static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd); > static void dp_netdev_pmd_unref(struct dp_netdev_pmd_thread *pmd); > static void dp_netdev_pmd_flow_flush(struct dp_netdev_pmd_thread *pmd); > @@ -913,7 +918,7 @@ create_dp_netdev(const char *name, const struct > dpif_class *class, > atomic_flag_clear(&dp->destroyed); > > ovs_mutex_init(&dp->port_mutex); > -cmap_init(&dp->ports); > +hmap_init(&dp->ports); > dp->port_seq = seq_create(); > fat_rwlock_init(&dp->upcall_rwlock); > > @@ -984,7 +989,7 @@ static void > dp_netdev_free(struct dp_netdev *dp) > OVS_REQUIRES(dp_netdev_mutex) > { > -struct dp_netdev_port *port; > +struct dp_netdev_port *port, *next; > > shash_find_and_delete(&dp_netdevs, dp->name); > > @@ -993,15 +998,14 @@ dp_netdev_free(struct dp_netdev *dp) > ovsthread_key_delete(dp->per_pmd_key); > > ovs_mutex_lock(&dp->port_mutex); > -CMAP_FOR_EACH (port, node, &dp->ports) { > -/* PMD threads are destroyed here. do_del_port() cannot quiesce */ > +HMAP_FOR_EACH_SAFE (port, next, node, &dp->ports) { > do_del_port(dp, port); > } > ovs_mutex_unlock(&dp->port_mutex); > cmap_destroy(&dp->poll_threads); > > seq_destroy(dp->port_seq); > -cmap_destroy(&dp->ports); > +hmap_destroy(&dp->ports); > ovs_mutex_destroy(&dp->port_mutex); > > /* Upcalls must be disabled at this point */ > @@ -1222,7 +1226,7 @@ do_ad
Re: [ovs-dev] [PATCH v8 10/16] dpif-netdev: Use hmap for ports.
On 20/04/2016 07:21, "Ilya Maximets" wrote: >On 20.04.2016 01:28, diproiettod at vmware.com (Daniele Di Proietto) >wrote: >> netdev objects are hard to use with RCU, because it's not possible to >> split removal and reclamation. Postponing the removal means that the >> port is not removed and cannot be readded immediately. Waiting for >> reclamation means introducing a quiescent state, and that may introduce >> subtle bugs, due to the RCU model we use in userspace. >> >> This commit changes the port container from cmap to hmap. 'port_mutex' >> must be held by readers and writers. This shouldn't have performace >> impact, as readers in the fast path use a thread local cache. >> >> Signed-off-by: Daniele Di Proietto >> --- >> lib/dpif-netdev.c | 96 >>+-- >> 1 file changed, 57 insertions(+), 39 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index bd2249e..8cc37e2 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -195,9 +195,10 @@ struct dp_netdev { >> >> /* Ports. >> * >> - * Protected by RCU. Take the mutex to add or remove ports. */ >> + * Any lookup into 'ports' or any access to the dp_netdev_ports >>found >> + * through 'ports' requires taking 'port_mutex'. */ >> struct ovs_mutex port_mutex; >> -struct cmap ports; >> +struct hmap ports; >> struct seq *port_seq; /* Incremented whenever a port >>changes. */ >> >> /* Protects access to ofproto-dpif-upcall interface during >>revalidator >> @@ -228,7 +229,8 @@ struct dp_netdev { >> }; >> >> static struct dp_netdev_port *dp_netdev_lookup_port(const struct >>dp_netdev *dp, >> -odp_port_t); >> +odp_port_t) >> +OVS_REQUIRES(&dp->port_mutex); > >OVS_REQUIRES(dp->port_mutex); >here and 2 times more below. I've changed them, thanks. I think the analyzer accepts both (a pointer or the object itself), but I prefer the syntax you suggested. > >> >> enum dp_stat_type { >> DP_STAT_EXACT_HIT, /* Packets that had an exact match >>(emc). */ >> @@ -248,7 +250,7 @@ enum pmd_cycles_counter_type { >> struct dp_netdev_port { >> odp_port_t port_no; >> struct netdev *netdev; >> -struct cmap_node node; /* Node in dp_netdev's 'ports'. */ >> +struct hmap_node node; /* Node in dp_netdev's 'ports'. */ >> struct netdev_saved_flags *sf; >> unsigned n_rxq; /* Number of elements in 'rxq' */ >> struct netdev_rxq **rxq; >> @@ -476,9 +478,11 @@ struct dpif_netdev { >> }; >> >> static int get_port_by_number(struct dp_netdev *dp, odp_port_t port_no, >> - struct dp_netdev_port **portp); >> + struct dp_netdev_port **portp) >> +OVS_REQUIRES(dp->port_mutex); >> static int get_port_by_name(struct dp_netdev *dp, const char *devname, >> -struct dp_netdev_port **portp); >> +struct dp_netdev_port **portp) >> +OVS_REQUIRES(dp->port_mutex); >> static void dp_netdev_free(struct dp_netdev *) >> OVS_REQUIRES(dp_netdev_mutex); >> static int do_add_port(struct dp_netdev *dp, const char *devname, >> @@ -522,7 +526,8 @@ dp_netdev_add_rxq_to_pmd(struct >>dp_netdev_pmd_thread *pmd, >> struct dp_netdev_port *port, struct >>netdev_rxq *rx); >> static struct dp_netdev_pmd_thread * >> dp_netdev_less_loaded_pmd_on_numa(struct dp_netdev *dp, int numa_id); >> -static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp); >> +static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp) >> +OVS_REQUIRES(dp->port_mutex); >> static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd); >> static void dp_netdev_pmd_unref(struct dp_netdev_pmd_thread *pmd); >> static void dp_netdev_pmd_flow_flush(struct dp_netdev_pmd_thread *pmd); >> @@ -913,7 +918,7 @@ create_dp_netdev(const char *name, const struct >>dpif_class *class, >> atomic_flag_clear(&dp->destroyed); >> >> ovs_mutex_init(&dp->port_mutex); >> -cmap_init(&dp->ports); >> +hmap_init(&dp->ports); >> dp->port_seq = seq_create(); >> fat_rwlock_init(&dp->upcall_rwlock); >> >> @@ -984,7 +989,7 @@ static void >> dp_netdev_free(struct dp_netdev *dp) >> OVS_REQUIRES(dp_netdev_mutex) >> { >> -struct dp_netdev_port *port; >> +struct dp_netdev_port *port, *next; >> >> shash_find_and_delete(&dp_netdevs, dp->name); >> >> @@ -993,15 +998,14 @@ dp_netdev_free(struct dp_netdev *dp) >> ovsthread_key_delete(dp->per_pmd_key); >> >> ovs_mutex_lock(&dp->port_mutex); >> -CMAP_FOR_EACH (port, node, &dp->ports) { >> -/* PMD threads are destroyed here. do_del_port() cannot >>quiesce */ >> +HMAP_FOR_EACH_SAFE (port, next, node, &dp->ports) { >> do_del_port(dp, port); >> } >> ov