Re: [ovs-dev] [PATCH V11 09/33] dpif: Save added ports in a port map for netdev flow api use

[Darrell Ball]

On 6/21/17, 4:27 AM, "ovs-dev-boun...@openvswitch.org on behalf of Roi Dayan" 
 wrote:



On 20/06/2017 03:07, Joe Stringer wrote:
> On 13 June 2017 at 08:03, 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 
>> Reviewed-by: Simon Horman 
>> Acked-by: Flavio Leitner 
>
> Hi Roi, Simon,
>
> It looks like this patch has introduced a regression with regards to
> clean shutdown of OVS. Unfortunately I wasn't able to discover this
> until now because the tree has been a bit unstable lately with respect
> to running the system-traffic tests and OVS cleaning up after itself.
> I've bisected it down now, though.
>
> To observe the issue, run:
> # make check-system-userspace TESTSUITEFLAGS="1"
> # ip link show ovs-netdev
>
> If you find that the ovs-netdev device exists, then you are observing
> the problem.
>
> Typically when the 'check-system-userspace' tests run successfully,
> they should initiate complete and proper shutdown of OVS and removal
> of related state via "ovs-appctl exit --cleanup", which you can find
> invoked via the shell function kill_ovs_vswitchd which is queued up in
> _OVS_VSWITCHD_START via the OVS "on_exit" function. However, since
> this patch was merged the test no longer cleans up properly after
> itself.
>
> Note that if you run any subsequent tests without cleaning up, then
> they will fail. For instance, if you run tests 1-5 you'll see that 1
> succeeds and 2-5 fail. Subsequent runs of any test will fail until you
> have removed the ovs-netdev device.
>
> Would you have some time to look into why ovs-vswitchd is not cleaning
> up this device during "ovs-appctl exit --cleanup" after this patch was
> applied?
>
> Thanks,
> Joe
>

We'll check it. Thanks

I hit this issue last week and worked around it after spending time
to figure out what had happened.

I sent a patch here:

https://patchwork.ozlabs.org/patch/780147/

Darrell

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=IGjDuLukWZUY9GwM5Fyb1aRjObV2fhSpVYinM7HfBE8=7VuqjZDLHGzgi40lHuo_AEFmZY4cHYl-h5TxMnEjR_4=
 


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

Re: [ovs-dev] [PATCH V11 09/33] dpif: Save added ports in a port map for netdev flow api use

[Roi Dayan]

On 20/06/2017 03:07, Joe Stringer wrote:

On 13 June 2017 at 08:03, 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 
Reviewed-by: Simon Horman 
Acked-by: Flavio Leitner 


Hi Roi, Simon,

It looks like this patch has introduced a regression with regards to
clean shutdown of OVS. Unfortunately I wasn't able to discover this
until now because the tree has been a bit unstable lately with respect
to running the system-traffic tests and OVS cleaning up after itself.
I've bisected it down now, though.

To observe the issue, run:
# make check-system-userspace TESTSUITEFLAGS="1"
# ip link show ovs-netdev

If you find that the ovs-netdev device exists, then you are observing
the problem.

Typically when the 'check-system-userspace' tests run successfully,
they should initiate complete and proper shutdown of OVS and removal
of related state via "ovs-appctl exit --cleanup", which you can find
invoked via the shell function kill_ovs_vswitchd which is queued up in
_OVS_VSWITCHD_START via the OVS "on_exit" function. However, since
this patch was merged the test no longer cleans up properly after
itself.

Note that if you run any subsequent tests without cleaning up, then
they will fail. For instance, if you run tests 1-5 you'll see that 1
succeeds and 2-5 fail. Subsequent runs of any test will fail until you
have removed the ovs-netdev device.

Would you have some time to look into why ovs-vswitchd is not cleaning
up this device during "ovs-appctl exit --cleanup" after this patch was
applied?

Thanks,
Joe



We'll check it. thanks
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V11 09/33] dpif: Save added ports in a port map for netdev flow api use

[Joe Stringer]

On 13 June 2017 at 08:03, 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 
> Reviewed-by: Simon Horman 
> Acked-by: Flavio Leitner 

Hi Roi, Simon,

It looks like this patch has introduced a regression with regards to
clean shutdown of OVS. Unfortunately I wasn't able to discover this
until now because the tree has been a bit unstable lately with respect
to running the system-traffic tests and OVS cleaning up after itself.
I've bisected it down now, though.

To observe the issue, run:
# make check-system-userspace TESTSUITEFLAGS="1"
# ip link show ovs-netdev

If you find that the ovs-netdev device exists, then you are observing
the problem.

Typically when the 'check-system-userspace' tests run successfully,
they should initiate complete and proper shutdown of OVS and removal
of related state via "ovs-appctl exit --cleanup", which you can find
invoked via the shell function kill_ovs_vswitchd which is queued up in
_OVS_VSWITCHD_START via the OVS "on_exit" function. However, since
this patch was merged the test no longer cleans up properly after
itself.

Note that if you run any subsequent tests without cleaning up, then
they will fail. For instance, if you run tests 1-5 you'll see that 1
succeeds and 2-5 fail. Subsequent runs of any test will fail until you
have removed the ovs-netdev device.

Would you have some time to look into why ovs-vswitchd is not cleaning
up this device during "ovs-appctl exit --cleanup" after this patch was
applied?

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

[ovs-dev] [PATCH V11 09/33] 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 
Reviewed-by: Simon Horman 
Acked-by: Flavio Leitner 
---
 lib/dpif.c   |  32 +++
 lib/dpif.h   |   2 +
 lib/netdev.c | 132 +++
 lib/netdev.h |   6 +++
 4 files changed, 172 insertions(+)

diff --git a/lib/dpif.c b/lib/dpif.c
index fe6a986..7756315 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -355,7 +355,28 @@ 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;
+
+if (!strcmp(dpif_port.type, "internal")) {
+continue;
+}
+
+err = netdev_open(dpif_port.name, dpif_port.type, );
+
+if (!err) {
+netdev_ports_insert(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);
 }
@@ -548,6 +569,15 @@ 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);
+
+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), _port);
+}
 } else {
 VLOG_WARN_RL(_rl, "%s: failed to add %s as port: %s",
  dpif_name(dpif), netdev_name, ovs_strerror(error));
@@ -572,6 +602,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_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 3204689..b0e1abd 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2127,6 +2127,138 @@ 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(_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 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, _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(_hmap_mutex);
+if (netdev_ports_lookup(dpif_port->port_no, obj)) {
+ovs_mutex_unlock(_hmap_mutex);
+return EEXIST;
+}
+
+data->netdev =