[ovs-dev] [cmap v2 2/2] dpif-netdev: Use cmap for ports.

Jarno Rajahalme jrajahalme at nicira.com
Tue May 20 19:25:50 UTC 2014


Ben,

Minor comments below,

  Jarno

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

On May 8, 2014, at 4:50 PM, Ben Pfaff <blp at nicira.com> wrote:

> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> lib/dpif-netdev.c |  163 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 82 insertions(+), 81 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 55712dd..b682876 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -32,6 +32,7 @@

(snip)

> @@ -671,7 +669,7 @@ dp_netdev_reload_pmd_threads(struct dp_netdev *dp)
> static int
> do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
>             odp_port_t port_no)
> -    OVS_REQ_WRLOCK(dp->port_rwlock)
> +    OVS_REQUIRES(dp->port_mutex)
> {
>     struct netdev_saved_flags *sf;
>     struct dp_netdev_port *port;
> @@ -733,7 +731,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
>     }
>     ovs_refcount_init(&port->ref_cnt);
> 
> -    hmap_insert(&dp->ports, &port->node, hash_int(odp_to_u32(port_no), 0));
> +    cmap_insert(&dp->ports, &port->node, hash_int(odp_to_u32(port_no), 0));

Could use the new hash_port_no() here.

>     seq_change(dp->port_seq);
> 
>     return 0;
> @@ -749,7 +747,7 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
>     odp_port_t port_no;
>     int error;
> 
> -    ovs_rwlock_wrlock(&dp->port_rwlock);
> +    ovs_mutex_lock(&dp->port_mutex);
>     dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
>     if (*port_nop != ODPP_NONE) {
>         port_no = *port_nop;
> @@ -762,7 +760,7 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
>         *port_nop = port_no;
>         error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
>     }
> -    ovs_rwlock_unlock(&dp->port_rwlock);
> +    ovs_mutex_unlock(&dp->port_mutex);
> 
>     return error;
> }
> @@ -773,9 +771,9 @@ dpif_netdev_port_del(struct dpif *dpif, odp_port_t port_no)
>     struct dp_netdev *dp = get_dp_netdev(dpif);
>     int error;
> 
> -    ovs_rwlock_wrlock(&dp->port_rwlock);
> +    ovs_mutex_lock(&dp->port_mutex);
>     error = port_no == ODPP_LOCAL ? EINVAL : do_del_port(dp, port_no);
> -    ovs_rwlock_unlock(&dp->port_rwlock);
> +    ovs_mutex_unlock(&dp->port_mutex);
> 
>     return error;
> }
> @@ -786,14 +784,18 @@ is_valid_port_number(odp_port_t port_no)
>     return port_no != ODPP_NONE;
> }
> 
> +static uint32_t
> +hash_port_no(odp_port_t port_no)
> +{
> +    return hash_int(odp_to_u32(port_no), 0);
> +}
> +

(snip)

> @@ -1783,10 +1784,9 @@ dpif_netdev_run(struct dpif *dpif)
> {
>     struct dp_netdev_port *port;
>     struct dp_netdev *dp = get_dp_netdev(dpif);
> +    struct cmap_cursor cursor;
> 
> -    ovs_rwlock_rdlock(&dp->port_rwlock);
> -

Unless it is OK to sometimes skip a port (due a concurrent insert, etc.) then we should still exclude writers here.

> -    HMAP_FOR_EACH (port, node, &dp->ports) {
> +    CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
>         if (!netdev_is_pmd(port->netdev)) {
>             int i;
> 
> @@ -1795,8 +1795,6 @@ dpif_netdev_run(struct dpif *dpif)
>             }
>         }
>     }
> -
> -    ovs_rwlock_unlock(&dp->port_rwlock);
> }
> 
> static void
> @@ -1804,10 +1802,9 @@ dpif_netdev_wait(struct dpif *dpif)
> {
>     struct dp_netdev_port *port;
>     struct dp_netdev *dp = get_dp_netdev(dpif);
> +    struct cmap_cursor cursor;
> 
> -    ovs_rwlock_rdlock(&dp->port_rwlock);
> -

Same here.

> -    HMAP_FOR_EACH (port, node, &dp->ports) {
> +    CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
>         if (!netdev_is_pmd(port->netdev)) {
>             int i;
> 
> @@ -1816,7 +1813,6 @@ dpif_netdev_wait(struct dpif *dpif)
>             }
>         }
>     }
> -    ovs_rwlock_unlock(&dp->port_rwlock);
> }
> 
> struct rxq_poll {
> @@ -1831,12 +1827,12 @@ pmd_load_queues(struct pmd_thread *f,
>     struct dp_netdev *dp = f->dp;
>     struct rxq_poll *poll_list = *ppoll_list;
>     struct dp_netdev_port *port;
> +    struct cmap_cursor cursor;
>     int id = f->id;
>     int index;
>     int i;
> 
>     /* Simple scheduler for netdev rx polling. */
> -    ovs_rwlock_rdlock(&dp->port_rwlock);

And here.

>     for (i = 0; i < poll_cnt; i++) {
>          port_unref(poll_list[i].port);
>     }
> @@ -1844,7 +1840,7 @@ pmd_load_queues(struct pmd_thread *f,
>     poll_cnt = 0;
>     index = 0;
> 
> -    HMAP_FOR_EACH (port, node, &f->dp->ports) {
> +    CMAP_FOR_EACH (port, node, &cursor, &f->dp->ports) {
>         if (netdev_is_pmd(port->netdev)) {
>             int i;
> 
> @@ -1862,7 +1858,6 @@ pmd_load_queues(struct pmd_thread *f,
>         }
>     }
> 
> -    ovs_rwlock_unlock(&dp->port_rwlock);
>     *ppoll_list = poll_list;
>     return poll_cnt;
> }

(snip)




More information about the dev mailing list