Re: [ovs-dev] [PATCH ovs V3 06/25] dpif: Save added ports in a port map for netdev flow api use

[Simon Horman]

On Wed, Feb 15, 2017 at 10:46:20AM +0200, Roi Dayan wrote:
> 
> 
> On 14/02/2017 17:52, Simon Horman wrote:
> >On Wed, Feb 08, 2017 at 05:29:19PM +0200, Roi Dayan wrote:
> >>From: Paul Blakey 
> >>
> >>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 
> >>Reviewed-by: Roi Dayan 
> >>---
> >> lib/dpif.c   |  25 
> >> lib/dpif.h   |   2 +
> >> lib/netdev.c | 121 
> >> +++
> >> lib/netdev.h |  15 
> >> 4 files changed, 163 insertions(+)
> >>
> >>diff --git a/lib/dpif.c b/lib/dpif.c
> >>index 57aa3c6..d4e4c0a 100644
> >>--- a/lib/dpif.c
> >>+++ b/lib/dpif.c
> >
> >...
> >
> >>@@ -545,6 +560,14 @@ dpif_port_add(struct dpif *dpif, struct netdev 
> >>*netdev, odp_port_t *port_nop)
> >> if (!error) {
> >> VLOG_DBG_RL(_rl, "%s: added %s as port %"PRIu32,
> >> dpif_name(dpif), netdev_name, port_no);
> >>+
> >>+/* temp dpif_port, will be cloned in netdev_hmap_port_add */
> >>+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_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif), _port);
> >
> >I wonder if it would be cleaner to get the dpif_port from the dpif API,
> >for example by providing a optionally NULL struct dpif_port * parameter to
> >dpif_class->port_add.
> >
> 
> I'm not sure I'm following you here. how would this be done?
> we want to same dpif_port and netdev resolution so we can use it later.
> right that we use it in dpif_class it self but other classes would want
> maybe to use this in the future so it's being handles in dpif_port_add.

I was just suggesting a minor cleanup as it seems a bit untidy to have
struct dpif_port data marshalled inline. My assumption was that
dpif->dpif_class->port_add(), which is called just above the code in
question, was doing the same marshalling and it could be leveraged.  I no
longer think that is the case.

...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V3 06/25] dpif: Save added ports in a port map for netdev flow api use

[Roi Dayan]

On 14/02/2017 17:52, Simon Horman wrote:

On Wed, Feb 08, 2017 at 05:29:19PM +0200, Roi Dayan wrote:

From: Paul Blakey 

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 
Reviewed-by: Roi Dayan 
---
 lib/dpif.c   |  25 
 lib/dpif.h   |   2 +
 lib/netdev.c | 121 +++
 lib/netdev.h |  15 
 4 files changed, 163 insertions(+)

diff --git a/lib/dpif.c b/lib/dpif.c
index 57aa3c6..d4e4c0a 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c


...


@@ -545,6 +560,14 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, 
odp_port_t *port_nop)
 if (!error) {
 VLOG_DBG_RL(_rl, "%s: added %s as port %"PRIu32,
 dpif_name(dpif), netdev_name, port_no);
+
+/* temp dpif_port, will be cloned in netdev_hmap_port_add */
+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_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif), _port);


I wonder if it would be cleaner to get the dpif_port from the dpif API,
for example by providing a optionally NULL struct dpif_port * parameter to
dpif_class->port_add.



I'm not sure I'm following you here. how would this be done?
we want to same dpif_port and netdev resolution so we can use it later.
right that we use it in dpif_class it self but other classes would want 
maybe to use this in the future so it's being handles in dpif_port_add.




 } else {
 VLOG_WARN_RL(_rl, "%s: failed to add %s as port: %s",
  dpif_name(dpif), netdev_name, ovs_strerror(error));


...


diff --git a/lib/netdev.h b/lib/netdev.h
index d7d4199..96300c4 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -181,6 +181,21 @@ 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_hmap_port_add(struct netdev *, const void *obj, struct dpif_port *);
+struct netdev *netdev_hmap_port_get(odp_port_t port, const void *obj);
+int netdev_hmap_port_del(odp_port_t port, const void *obj);
+struct netdev_flow_dump **netdev_ports_flow_dumps_create(const void *obj,
+ int *ports);
+void netdev_ports_flow_flush(const void *obj);


Two functions function above are defined in the following patch.
Probably their definitions should go there too.



thanks. will be fixed.


+int netdev_ports_flow_del(const void *obj, const ovs_u128 *ufid,
+  struct dpif_flow_stats *stats);


Likewise, the above patch seems to be defined in patch 17 of this series.


+int netdev_ports_flow_get(const void *obj, struct match *match,
+  struct nlattr **actions,
+  struct dpif_flow_stats *stats,
+  const ovs_u128 *ufid, struct ofpbuf *buf);


The above function is declared in this patch, used in patch 12 and
defined in patch 17. This does not seem correct from a bisection point of
view.



thanks. same for netdev_ports_flow_del. will be fixed.


+odp_port_t netdev_hmap_port_get_byifidx(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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V3 06/25] dpif: Save added ports in a port map for netdev flow api use

[Simon Horman]

On Wed, Feb 08, 2017 at 05:29:19PM +0200, Roi Dayan wrote:
> From: Paul Blakey 
> 
> 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 
> Reviewed-by: Roi Dayan 
> ---
>  lib/dpif.c   |  25 
>  lib/dpif.h   |   2 +
>  lib/netdev.c | 121 
> +++
>  lib/netdev.h |  15 
>  4 files changed, 163 insertions(+)
> 
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 57aa3c6..d4e4c0a 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c

...

> @@ -545,6 +560,14 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, 
> odp_port_t *port_nop)
>  if (!error) {
>  VLOG_DBG_RL(_rl, "%s: added %s as port %"PRIu32,
>  dpif_name(dpif), netdev_name, port_no);
> +
> +/* temp dpif_port, will be cloned in netdev_hmap_port_add */
> +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_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif), _port);

I wonder if it would be cleaner to get the dpif_port from the dpif API,
for example by providing a optionally NULL struct dpif_port * parameter to
dpif_class->port_add.

>  } else {
>  VLOG_WARN_RL(_rl, "%s: failed to add %s as port: %s",
>   dpif_name(dpif), netdev_name, ovs_strerror(error));

...

> diff --git a/lib/netdev.h b/lib/netdev.h
> index d7d4199..96300c4 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -181,6 +181,21 @@ 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_hmap_port_add(struct netdev *, const void *obj, struct dpif_port 
> *);
> +struct netdev *netdev_hmap_port_get(odp_port_t port, const void *obj);
> +int netdev_hmap_port_del(odp_port_t port, const void *obj);
> +struct netdev_flow_dump **netdev_ports_flow_dumps_create(const void *obj,
> + int *ports);
> +void netdev_ports_flow_flush(const void *obj);

Two functions function above are defined in the following patch.
Probably their definitions should go there too.

> +int netdev_ports_flow_del(const void *obj, const ovs_u128 *ufid,
> +  struct dpif_flow_stats *stats);

Likewise, the above patch seems to be defined in patch 17 of this series.

> +int netdev_ports_flow_get(const void *obj, struct match *match,
> +  struct nlattr **actions,
> +  struct dpif_flow_stats *stats,
> +  const ovs_u128 *ufid, struct ofpbuf *buf);

The above function is declared in this patch, used in patch 12 and
defined in patch 17. This does not seem correct from a bisection point of
view.

> +odp_port_t netdev_hmap_port_get_byifidx(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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovs V3 06/25] dpif: Save added ports in a port map for netdev flow api use

[Roi Dayan]

From: Paul Blakey 

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 
Reviewed-by: Roi Dayan 
---
 lib/dpif.c   |  25 
 lib/dpif.h   |   2 +
 lib/netdev.c | 121 +++
 lib/netdev.h |  15 
 4 files changed, 163 insertions(+)

diff --git a/lib/dpif.c b/lib/dpif.c
index 57aa3c6..d4e4c0a 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -352,7 +352,22 @@ 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, );
 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(_port, _dump, dpif) {
+struct netdev *netdev;
+int err = netdev_open(dpif_port.name, dpif_port.type, );
+
+if (!err) {
+netdev_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif), _port);
+netdev_close(netdev);
+} else {
+VLOG_WARN("could not open netdev %s type %s", name, type);
+}
+}
 } else {
 dp_class_unref(registered_class);
 }
@@ -545,6 +560,14 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, 
odp_port_t *port_nop)
 if (!error) {
 VLOG_DBG_RL(_rl, "%s: added %s as port %"PRIu32,
 dpif_name(dpif), netdev_name, port_no);
+
+/* temp dpif_port, will be cloned in netdev_hmap_port_add */
+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_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif), _port);
 } else {
 VLOG_WARN_RL(_rl, "%s: failed to add %s as port: %s",
  dpif_name(dpif), netdev_name, ovs_strerror(error));
@@ -569,6 +592,8 @@ dpif_port_del(struct dpif *dpif, odp_port_t port_no)
 if (!error) {
 VLOG_DBG_RL(_rl, "%s: port_del(%"PRIu32")",
 dpif_name(dpif), port_no);
+
+netdev_hmap_port_del(port_no, DPIF_HMAP_KEY(dpif));
 } else {
 log_operation(dpif, "port_del", error);
 }
diff --git a/lib/dpif.h b/lib/dpif.h
index d5b4b58..44cbe95 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -400,6 +400,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 d5935c3..792ab8c 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2104,6 +2104,127 @@ 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(_to_netdev);
+static struct hmap ifindex_to_port OVS_GUARDED_BY(netdev_hmap_mutex)
+= HMAP_INITIALIZER(_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 int
+netdev_hmap_port_del_locked(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, _to_netdev) {
+if (data->obj == obj && data->dpif_port.port_no == port_no) {
+break;
+}
+}
+
+if (data) {
+dpif_port_destroy(>dpif_port);
+netdev_close(data->netdev); /* unref and possibly close */
+hmap_remove(_to_netdev, >node);
+free(data);
+return 0;
+}
+
+return ENOENT;
+}
+
+int
+netdev_hmap_port_add(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 = xzalloc(sizeof *data);
+struct ifindex_to_port_data *ifidx = xzalloc(sizeof *ifidx);
+
+ovs_mutex_lock(_hmap_mutex);
+netdev_hmap_port_del_locked(dpif_port->port_no, obj);
+
+data->netdev = netdev_ref(netdev);
+data->obj = obj;
+dpif_port_clone(>dpif_port, dpif_port);
+
+ifidx->ifindex = netdev_get_ifindex(netdev);
+ifidx->port = dpif_port->port_no;
+
+hmap_insert(_to_netdev, >node, hash);
+hmap_insert(_to_port, >node, ifidx->ifindex);
+