diff mbox

[ovs-dev,ovs,V8,05/26] dpif: Save added ports in a port map for netdev flow api use

Message ID 1493824097-47495-6-git-send-email-roid@mellanox.com
State Changes Requested
Headers show

Commit Message

Roi Dayan May 3, 2017, 3:07 p.m. UTC
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>
---
 lib/dpif.c   |  30 ++++++++++++++
 lib/dpif.h   |   2 +
 lib/netdev.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/netdev.h |   6 +++
 4 files changed, 169 insertions(+)

Comments

Simon Horman May 8, 2017, 12:49 p.m. UTC | #1
On Wed, May 03, 2017 at 06:07:56PM +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>

...

> diff --git a/lib/netdev.h b/lib/netdev.h
> index 7435fdf..9aa7e5e 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
>  extern bool netdev_flow_api_enabled;
>  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 {

This patch seems to only partially address the review provided
by Joe Stringer for v7. In particular:

* netdev_ports_get() -> netdev_ports_lookup()
* Feedback regarding 'obj' being a not particularly clear abstraction.
Roi Dayan May 16, 2017, 9:16 a.m. UTC | #2
On 08/05/2017 15:49, Simon Horman wrote:
> On Wed, May 03, 2017 at 06:07:56PM +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>
>
> ...
>
>> diff --git a/lib/netdev.h b/lib/netdev.h
>> index 7435fdf..9aa7e5e 100644
>> --- a/lib/netdev.h
>> +++ b/lib/netdev.h
>> @@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
>>  extern bool netdev_flow_api_enabled;
>>  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 {
>
> This patch seems to only partially address the review provided
> by Joe Stringer for v7. In particular:
>
> * netdev_ports_get() -> netdev_ports_lookup()
> * Feedback regarding 'obj' being a not particularly clear abstraction.
>

we did refactor all functions to have prefix netdev_ports_*
there are both functions netdev_ports_get() and netdev_ports_lookup().
did I miss something?

about 'obj', I mentioned this in the changelog that it's left out for 
now and could be done in followup commit. is it ok?
Simon Horman May 19, 2017, 8:46 p.m. UTC | #3
On Tue, May 16, 2017 at 12:16:46PM +0300, Roi Dayan wrote:
> 
> 
> On 08/05/2017 15:49, Simon Horman wrote:
> >On Wed, May 03, 2017 at 06:07:56PM +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>
> >
> >...
> >
> >>diff --git a/lib/netdev.h b/lib/netdev.h
> >>index 7435fdf..9aa7e5e 100644
> >>--- a/lib/netdev.h
> >>+++ b/lib/netdev.h
> >>@@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
> >> extern bool netdev_flow_api_enabled;
> >> 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 {
> >
> >This patch seems to only partially address the review provided
> >by Joe Stringer for v7. In particular:
> >
> >* netdev_ports_get() -> netdev_ports_lookup()
> >* Feedback regarding 'obj' being a not particularly clear abstraction.
> >
> 
> we did refactor all functions to have prefix netdev_ports_*
> there are both functions netdev_ports_get() and netdev_ports_lookup().
> did I miss something?

Thanks, I see that now. Looks good.

> about 'obj', I mentioned this in the changelog that it's left out for now
> and could be done in followup commit. is it ok?

Yes, that is fine by me.
diff mbox

Patch

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;
+            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);
+            }
+        }
     } 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 086b067..1e0e991 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2104,6 +2104,137 @@  netdev_init_flow_api(struct netdev *netdev)
             : EOPNOTSUPP);
 }
 
+/* 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)
+{
+    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;
+}
+
 bool netdev_flow_api_enabled = false;
 
 #ifdef __linux__
diff --git a/lib/netdev.h b/lib/netdev.h
index 7435fdf..9aa7e5e 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -181,6 +181,12 @@  int netdev_init_flow_api(struct netdev *);
 extern bool netdev_flow_api_enabled;
 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 {