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
