[ovs-dev,V9,08/31] dpif: Save added ports in a port map for netdev flow api use

From: Paul Blakey <paulb@mellanox.com>

To use netdev flow offloading api, dpifs needs to iterate over
added ports. This addition inserts the added dpif ports in a hash map,
The map will also be used to translate dpif ports to netdevs.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/dpif.c   |  30 ++++++++++++++
 lib/dpif.h   |   2 +
 lib/netdev.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/netdev.h |   6 +++
 4 files changed, 169 insertions(+)

On Sun, May 28, 2017 at 02:59:50PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> To use netdev flow offloading api, dpifs needs to iterate over
> added ports. This addition inserts the added dpif ports in a hash map,
> The map will also be used to translate dpif ports to netdevs.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> Acked-by: Flavio Leitner <fbl@sysclose.org>

Thanks, this looks good to me.  As it has been acked I would I would be
happy to apply it once earlier in the patches in the series have been
(reviewed and) applied.

On Sun, May 28, 2017 at 02:59:50PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> To use netdev flow offloading api, dpifs needs to iterate over
> added ports. This addition inserts the added dpif ports in a hash map,
> The map will also be used to translate dpif ports to netdevs.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> Acked-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  lib/dpif.c   |  30 ++++++++++++++
>  lib/dpif.h   |   2 +
>  lib/netdev.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/netdev.h |   6 +++
>  4 files changed, 169 insertions(+)
> 
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 4066f9c..33037a9 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -355,7 +355,26 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
>      error = registered_class->dpif_class->open(registered_class->dpif_class,
>                                                 name, create, &dpif);
>      if (!error) {
> +        struct dpif_port_dump port_dump;
> +        struct dpif_port dpif_port;
> +
>          ovs_assert(dpif->dpif_class == registered_class->dpif_class);
> +
> +        DPIF_PORT_FOR_EACH(&dpif_port, &port_dump, dpif) {
> +            if (!strcmp(dpif_port.type, "internal")) {
> +                continue;
> +            }
> +
> +            struct netdev *netdev;

This and the err below should be below the DPIF_PORT_FOR_EACH() I think.

> +            int err = netdev_open(dpif_port.name, dpif_port.type, &netdev);
> +
> +            if (!err) {
> +                netdev_ports_insert(netdev, DPIF_HMAP_KEY(dpif), &dpif_port);
> +                netdev_close(netdev);
> +            } else {
> +                VLOG_WARN("could not open netdev %s type %s", name, type);
> +            }

So, if there is an error, it fails to map the device which will affect
offloading, but not OVS in general, correct?



> +        }
>      } else {
>          dp_class_unref(registered_class);
>      }
> @@ -548,6 +567,15 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
>      if (!error) {
>          VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
>                      dpif_name(dpif), netdev_name, port_no);
> +
> +        if (strcmp(netdev_get_type(netdev), "internal")) {
> +            struct dpif_port dpif_port;
> +
> +            dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev));
> +            dpif_port.name = CONST_CAST(char *, netdev_name);
> +            dpif_port.port_no = port_no;
> +            netdev_ports_insert(netdev, DPIF_HMAP_KEY(dpif), &dpif_port);
> +        }
>      } else {
>          VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
>                       dpif_name(dpif), netdev_name, ovs_strerror(error));
> @@ -572,6 +600,8 @@ dpif_port_del(struct dpif *dpif, odp_port_t port_no)
>      if (!error) {
>          VLOG_DBG_RL(&dpmsg_rl, "%s: port_del(%"PRIu32")",
>                      dpif_name(dpif), port_no);
> +
> +        netdev_ports_remove(port_no, DPIF_HMAP_KEY(dpif));
>      } else {
>          log_operation(dpif, "port_del", error);
>      }
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 5d49b11..81376c0 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -401,6 +401,8 @@
>  extern "C" {
>  #endif
>  
> +#define DPIF_HMAP_KEY(x) ((x)->dpif_class)
> +
>  struct dpif;
>  struct dpif_class;
>  struct dpif_flow;
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 19d809b..88a0a61 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -2108,6 +2108,137 @@ netdev_is_flow_api_enabled(void)
>      return netdev_flow_api_enabled;
>  }
>  
> +/* Protects below port hashmaps. */
> +static struct ovs_mutex netdev_hmap_mutex = OVS_MUTEX_INITIALIZER;
> +
> +static struct hmap port_to_netdev OVS_GUARDED_BY(netdev_hmap_mutex)
> +    = HMAP_INITIALIZER(&port_to_netdev);
> +static struct hmap ifindex_to_port OVS_GUARDED_BY(netdev_hmap_mutex)
> +    = HMAP_INITIALIZER(&ifindex_to_port);
> +
> +struct port_to_netdev_data {
> +    struct hmap_node node;
> +    struct netdev *netdev;
> +    struct dpif_port dpif_port;
> +    const void *obj;
> +};
> +
> +struct ifindex_to_port_data {
> +    struct hmap_node node;
> +    int ifindex;
> +    odp_port_t port;
> +};
> +
> +static struct port_to_netdev_data *
> +netdev_ports_lookup(odp_port_t port_no, const void *obj)

   OVS_REQUIRES(netdev_hmap_mutex) ?


> +{
> +    size_t hash = hash_int(odp_to_u32(port_no), hash_pointer(obj, 0));
> +    struct port_to_netdev_data *data;
> +
> +    HMAP_FOR_EACH_WITH_HASH(data, node, hash, &port_to_netdev) {
> +            if (data->obj == obj && data->dpif_port.port_no == port_no) {
> +                return data;
> +            }
> +    }
> +    return NULL;
> +}
> +
> +int
> +netdev_ports_insert(struct netdev *netdev, const void *obj,
> +                    struct dpif_port *dpif_port)
> +{
> +    size_t hash = hash_int(odp_to_u32(dpif_port->port_no),
> +                           hash_pointer(obj, 0));
> +    struct port_to_netdev_data *data;
> +    struct ifindex_to_port_data *ifidx;
> +    int ifindex = netdev_get_ifindex(netdev);
> +
> +    if (ifindex < 0) {
> +        return ENODEV;
> +    }
> +
> +    data = xzalloc(sizeof *data);
> +    ifidx = xzalloc(sizeof *ifidx);
> +
> +    ovs_mutex_lock(&netdev_hmap_mutex);
> +    if (netdev_ports_lookup(dpif_port->port_no, obj)) {
> +        ovs_mutex_unlock(&netdev_hmap_mutex);
> +        return EEXIST;
> +    }
> +
> +    data->netdev = netdev_ref(netdev);
> +    data->obj = obj;
> +    dpif_port_clone(&data->dpif_port, dpif_port);
> +
> +    ifidx->ifindex = ifindex;
> +    ifidx->port = dpif_port->port_no;
> +
> +    hmap_insert(&port_to_netdev, &data->node, hash);
> +    hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex);
> +    ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> +    netdev_init_flow_api(netdev);
> +
> +    return 0;
> +}
> +
> +struct netdev *
> +netdev_ports_get(odp_port_t port_no, const void *obj)
> +{
> +    struct port_to_netdev_data *data;
> +    struct netdev *ret = NULL;
> +
> +    ovs_mutex_lock(&netdev_hmap_mutex);
> +    data = netdev_ports_lookup(port_no, obj);
> +    if (data) {
> +        ret = netdev_ref(data->netdev);
> +    }
> +    ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> +    return ret;
> +}
> +
> +int
> +netdev_ports_remove(odp_port_t port_no, const void *obj)
> +{
> +    struct port_to_netdev_data *data;
> +    int ret = ENOENT;
> +
> +    ovs_mutex_lock(&netdev_hmap_mutex);
> +
> +    data = netdev_ports_lookup(port_no, obj);
> +
> +    if (data) {
> +        dpif_port_destroy(&data->dpif_port);
> +        netdev_close(data->netdev); /* unref and possibly close */
> +        hmap_remove(&port_to_netdev, &data->node);
> +        free(data);
> +        ret = 0;
> +    }
> +
> +    ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> +    return ret;
> +}
> +
> +odp_port_t
> +netdev_ifindex_to_odp_port(int ifindex)
> +{
> +    struct ifindex_to_port_data *data;
> +    odp_port_t ret = 0;
> +
> +    ovs_mutex_lock(&netdev_hmap_mutex);
> +    HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) {
> +            if (data->ifindex == ifindex) {
> +                ret = data->port;
> +                break;
> +            }
> +    }
> +    ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> +    return ret;
> +}
> +
>  #ifdef __linux__
>  void
>  netdev_set_flow_api_enabled(const struct smap *ovs_other_config)
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 696f0f4..7628397 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
>  bool netdev_is_flow_api_enabled(void);
>  void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
>  
> +struct dpif_port;
> +int netdev_ports_insert(struct netdev *, const void *obj, struct dpif_port *);
> +struct netdev *netdev_ports_get(odp_port_t port, const void *obj);
> +int netdev_ports_remove(odp_port_t port, const void *obj);
> +odp_port_t netdev_ifindex_to_odp_port(int ifindex);
> +
>  /* native tunnel APIs */
>  /* Structure to pass parameters required to build a tunnel header. */
>  struct netdev_tnl_build_header_params {
> -- 
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 02/06/2017 16:45, Flavio Leitner wrote:
> On Sun, May 28, 2017 at 02:59:50PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb@mellanox.com>
>>
>> To use netdev flow offloading api, dpifs needs to iterate over
>> added ports. This addition inserts the added dpif ports in a hash map,
>> The map will also be used to translate dpif ports to netdevs.
>>
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
>> Acked-by: Flavio Leitner <fbl@sysclose.org>
>> ---
>>  lib/dpif.c   |  30 ++++++++++++++
>>  lib/dpif.h   |   2 +
>>  lib/netdev.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/netdev.h |   6 +++
>>  4 files changed, 169 insertions(+)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 4066f9c..33037a9 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -355,7 +355,26 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
>>      error = registered_class->dpif_class->open(registered_class->dpif_class,
>>                                                 name, create, &dpif);
>>      if (!error) {
>> +        struct dpif_port_dump port_dump;
>> +        struct dpif_port dpif_port;
>> +
>>          ovs_assert(dpif->dpif_class == registered_class->dpif_class);
>> +
>> +        DPIF_PORT_FOR_EACH(&dpif_port, &port_dump, dpif) {
>> +            if (!strcmp(dpif_port.type, "internal")) {
>> +                continue;
>> +            }
>> +
>> +            struct netdev *netdev;
>
> This and the err below should be below the DPIF_PORT_FOR_EACH() I think.

you mean the definitions to be at the start of the block, right? fixed.

>
>> +            int err = netdev_open(dpif_port.name, dpif_port.type, &netdev);
>> +
>> +            if (!err) {
>> +                netdev_ports_insert(netdev, DPIF_HMAP_KEY(dpif), &dpif_port);
>> +                netdev_close(netdev);
>> +            } else {
>> +                VLOG_WARN("could not open netdev %s type %s", name, type);
>> +            }
>
> So, if there is an error, it fails to map the device which will affect
> offloading, but not OVS in general, correct?

correct.

>
>
>
>> +        }
>>      } else {
>>          dp_class_unref(registered_class);
>>      }
>> @@ -548,6 +567,15 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
>>      if (!error) {
>>          VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
>>                      dpif_name(dpif), netdev_name, port_no);
>> +
>> +        if (strcmp(netdev_get_type(netdev), "internal")) {
>> +            struct dpif_port dpif_port;
>> +
>> +            dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev));
>> +            dpif_port.name = CONST_CAST(char *, netdev_name);
>> +            dpif_port.port_no = port_no;
>> +            netdev_ports_insert(netdev, DPIF_HMAP_KEY(dpif), &dpif_port);
>> +        }
>>      } else {
>>          VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
>>                       dpif_name(dpif), netdev_name, ovs_strerror(error));
>> @@ -572,6 +600,8 @@ dpif_port_del(struct dpif *dpif, odp_port_t port_no)
>>      if (!error) {
>>          VLOG_DBG_RL(&dpmsg_rl, "%s: port_del(%"PRIu32")",
>>                      dpif_name(dpif), port_no);
>> +
>> +        netdev_ports_remove(port_no, DPIF_HMAP_KEY(dpif));
>>      } else {
>>          log_operation(dpif, "port_del", error);
>>      }
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index 5d49b11..81376c0 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -401,6 +401,8 @@
>>  extern "C" {
>>  #endif
>>
>> +#define DPIF_HMAP_KEY(x) ((x)->dpif_class)
>> +
>>  struct dpif;
>>  struct dpif_class;
>>  struct dpif_flow;
>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index 19d809b..88a0a61 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -2108,6 +2108,137 @@ netdev_is_flow_api_enabled(void)
>>      return netdev_flow_api_enabled;
>>  }
>>
>> +/* Protects below port hashmaps. */
>> +static struct ovs_mutex netdev_hmap_mutex = OVS_MUTEX_INITIALIZER;
>> +
>> +static struct hmap port_to_netdev OVS_GUARDED_BY(netdev_hmap_mutex)
>> +    = HMAP_INITIALIZER(&port_to_netdev);
>> +static struct hmap ifindex_to_port OVS_GUARDED_BY(netdev_hmap_mutex)
>> +    = HMAP_INITIALIZER(&ifindex_to_port);
>> +
>> +struct port_to_netdev_data {
>> +    struct hmap_node node;
>> +    struct netdev *netdev;
>> +    struct dpif_port dpif_port;
>> +    const void *obj;
>> +};
>> +
>> +struct ifindex_to_port_data {
>> +    struct hmap_node node;
>> +    int ifindex;
>> +    odp_port_t port;
>> +};
>> +
>> +static struct port_to_netdev_data *
>> +netdev_ports_lookup(odp_port_t port_no, const void *obj)
>
>    OVS_REQUIRES(netdev_hmap_mutex) ?
>

added. thanks

>
>> +{
>> +    size_t hash = hash_int(odp_to_u32(port_no), hash_pointer(obj, 0));
>> +    struct port_to_netdev_data *data;
>> +
>> +    HMAP_FOR_EACH_WITH_HASH(data, node, hash, &port_to_netdev) {
>> +            if (data->obj == obj && data->dpif_port.port_no == port_no) {
>> +                return data;
>> +            }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +int
>> +netdev_ports_insert(struct netdev *netdev, const void *obj,
>> +                    struct dpif_port *dpif_port)
>> +{
>> +    size_t hash = hash_int(odp_to_u32(dpif_port->port_no),
>> +                           hash_pointer(obj, 0));
>> +    struct port_to_netdev_data *data;
>> +    struct ifindex_to_port_data *ifidx;
>> +    int ifindex = netdev_get_ifindex(netdev);
>> +
>> +    if (ifindex < 0) {
>> +        return ENODEV;
>> +    }
>> +
>> +    data = xzalloc(sizeof *data);
>> +    ifidx = xzalloc(sizeof *ifidx);
>> +
>> +    ovs_mutex_lock(&netdev_hmap_mutex);
>> +    if (netdev_ports_lookup(dpif_port->port_no, obj)) {
>> +        ovs_mutex_unlock(&netdev_hmap_mutex);
>> +        return EEXIST;
>> +    }
>> +
>> +    data->netdev = netdev_ref(netdev);
>> +    data->obj = obj;
>> +    dpif_port_clone(&data->dpif_port, dpif_port);
>> +
>> +    ifidx->ifindex = ifindex;
>> +    ifidx->port = dpif_port->port_no;
>> +
>> +    hmap_insert(&port_to_netdev, &data->node, hash);
>> +    hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex);
>> +    ovs_mutex_unlock(&netdev_hmap_mutex);
>> +
>> +    netdev_init_flow_api(netdev);
>> +
>> +    return 0;
>> +}
>> +
>> +struct netdev *
>> +netdev_ports_get(odp_port_t port_no, const void *obj)
>> +{
>> +    struct port_to_netdev_data *data;
>> +    struct netdev *ret = NULL;
>> +
>> +    ovs_mutex_lock(&netdev_hmap_mutex);
>> +    data = netdev_ports_lookup(port_no, obj);
>> +    if (data) {
>> +        ret = netdev_ref(data->netdev);
>> +    }
>> +    ovs_mutex_unlock(&netdev_hmap_mutex);
>> +
>> +    return ret;
>> +}
>> +
>> +int
>> +netdev_ports_remove(odp_port_t port_no, const void *obj)
>> +{
>> +    struct port_to_netdev_data *data;
>> +    int ret = ENOENT;
>> +
>> +    ovs_mutex_lock(&netdev_hmap_mutex);
>> +
>> +    data = netdev_ports_lookup(port_no, obj);
>> +
>> +    if (data) {
>> +        dpif_port_destroy(&data->dpif_port);
>> +        netdev_close(data->netdev); /* unref and possibly close */
>> +        hmap_remove(&port_to_netdev, &data->node);
>> +        free(data);
>> +        ret = 0;
>> +    }
>> +
>> +    ovs_mutex_unlock(&netdev_hmap_mutex);
>> +
>> +    return ret;
>> +}
>> +
>> +odp_port_t
>> +netdev_ifindex_to_odp_port(int ifindex)
>> +{
>> +    struct ifindex_to_port_data *data;
>> +    odp_port_t ret = 0;
>> +
>> +    ovs_mutex_lock(&netdev_hmap_mutex);
>> +    HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) {
>> +            if (data->ifindex == ifindex) {
>> +                ret = data->port;
>> +                break;
>> +            }
>> +    }
>> +    ovs_mutex_unlock(&netdev_hmap_mutex);
>> +
>> +    return ret;
>> +}
>> +
>>  #ifdef __linux__
>>  void
>>  netdev_set_flow_api_enabled(const struct smap *ovs_other_config)
>> diff --git a/lib/netdev.h b/lib/netdev.h
>> index 696f0f4..7628397 100644
>> --- a/lib/netdev.h
>> +++ b/lib/netdev.h
>> @@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
>>  bool netdev_is_flow_api_enabled(void);
>>  void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
>>
>> +struct dpif_port;
>> +int netdev_ports_insert(struct netdev *, const void *obj, struct dpif_port *);
>> +struct netdev *netdev_ports_get(odp_port_t port, const void *obj);
>> +int netdev_ports_remove(odp_port_t port, const void *obj);
>> +odp_port_t netdev_ifindex_to_odp_port(int ifindex);
>> +
>>  /* native tunnel APIs */
>>  /* Structure to pass parameters required to build a tunnel header. */
>>  struct netdev_tnl_build_header_params {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>