Re: [ovs-dev] [PATCH v2 2/2] netdev: Eliminate redundant ifindex mapping.

2017-11-15 Thread Ben Pfaff
On Wed, Nov 15, 2017 at 06:57:01AM -0800, William Tu wrote:
> On Tue, Nov 14, 2017 at 10:15 AM, Ben Pfaff  wrote:
> > Until now, the code for mapping ODP port number to ifindexes and vice versa
> > has maintained two completely separate data structures, one for each
> > direction.  It was possible for the two mappings to become out of sync
> > with each other since either one could change independently.  This commit
> > merges them into a single data structure (with two indexes), which at least
> > means that if one is removed then the other is as well.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> 
> Looks good to me.
> 
> Acked-by: William Tu 

Thanks William, I applied these patches to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v2 2/2] netdev: Eliminate redundant ifindex mapping.

2017-11-15 Thread William Tu
On Tue, Nov 14, 2017 at 10:15 AM, Ben Pfaff  wrote:
> Until now, the code for mapping ODP port number to ifindexes and vice versa
> has maintained two completely separate data structures, one for each
> direction.  It was possible for the two mappings to become out of sync
> with each other since either one could change independently.  This commit
> merges them into a single data structure (with two indexes), which at least
> means that if one is removed then the other is as well.
>
> Signed-off-by: Ben Pfaff 
> ---

Looks good to me.

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


[ovs-dev] [PATCH v2 2/2] netdev: Eliminate redundant ifindex mapping.

2017-11-14 Thread Ben Pfaff
Until now, the code for mapping ODP port number to ifindexes and vice versa
has maintained two completely separate data structures, one for each
direction.  It was possible for the two mappings to become out of sync
with each other since either one could change independently.  This commit
merges them into a single data structure (with two indexes), which at least
means that if one is removed then the other is as well.

Signed-off-by: Ben Pfaff 
---
 lib/netdev.c | 71 +++-
 1 file changed, 17 insertions(+), 54 deletions(-)

diff --git a/lib/netdev.c b/lib/netdev.c
index 8dd35864d7cb..c52680659e3f 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2166,17 +2166,12 @@ 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 hmap_node portno_node; /* By (dpif_class, dpif_port.port_no). */
+struct hmap_node ifindex_node; /* By (dpif_class, ifindex). */
 struct netdev *netdev;
 struct dpif_port dpif_port;
 const struct dpif_class *dpif_class;
-};
-
-struct ifindex_to_port_data {
-struct hmap_node node;
 int ifindex;
-odp_port_t port;
-const struct dpif_class *dpif_class;
 };
 
 static uint32_t
@@ -2191,7 +2186,7 @@ netdev_ports_lookup(odp_port_t port_no, const struct 
dpif_class *dpif_class)
 {
 struct port_to_netdev_data *data;
 
-HMAP_FOR_EACH_WITH_HASH (data, node,
+HMAP_FOR_EACH_WITH_HASH (data, portno_node,
  netdev_ports_hash(port_no, dpif_class),
  &port_to_netdev) {
 if (data->dpif_class == dpif_class
@@ -2207,7 +2202,6 @@ netdev_ports_insert(struct netdev *netdev, const struct 
dpif_class *dpif_class,
 struct dpif_port *dpif_port)
 {
 struct port_to_netdev_data *data;
-struct ifindex_to_port_data *ifidx;
 int ifindex = netdev_get_ifindex(netdev);
 
 if (ifindex < 0) {
@@ -2224,15 +2218,11 @@ netdev_ports_insert(struct netdev *netdev, const struct 
dpif_class *dpif_class,
 data->netdev = netdev_ref(netdev);
 data->dpif_class = dpif_class;
 dpif_port_clone(&data->dpif_port, dpif_port);
+data->ifindex = ifindex;
 
-ifidx = xzalloc(sizeof *ifidx);
-ifidx->ifindex = ifindex;
-ifidx->port = dpif_port->port_no;
-ifidx->dpif_class = dpif_class;
-
-hmap_insert(&port_to_netdev, &data->node,
+hmap_insert(&port_to_netdev, &data->portno_node,
 netdev_ports_hash(dpif_port->port_no, dpif_class));
-hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex);
+hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
 ovs_mutex_unlock(&netdev_hmap_mutex);
 
 netdev_init_flow_api(netdev);
@@ -2265,38 +2255,11 @@ netdev_ports_remove(odp_port_t port_no, const struct 
dpif_class *dpif_class)
 ovs_mutex_lock(&netdev_hmap_mutex);
 
 data = netdev_ports_lookup(port_no, dpif_class);
-
 if (data) {
-int ifindex = netdev_get_ifindex(data->netdev);
-struct ifindex_to_port_data *ifidx = NULL;
-
-if (ifindex > 0) {
-HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, &ifindex_to_port) {
-if (ifidx->port == port_no) {
-hmap_remove(&ifindex_to_port, &ifidx->node);
-free(ifidx);
-break;
-}
-}
-ovs_assert(ifidx);
-} else {
-/* case where the interface is already deleted form the datapath
- * (e.g. tunctl -d or ip tuntap del), then the ifindex is not
- * valid anymore. Traverse the HMAP for the port number. */
-HMAP_FOR_EACH (ifidx, node, &ifindex_to_port) {
-if (ifidx->port == port_no &&
-ifidx->dpif_class == dpif_class) {
-hmap_remove(&ifindex_to_port, &ifidx->node);
-free(ifidx);
-break;
-}
-}
-ovs_assert(ifidx);
-}
-
 dpif_port_destroy(&data->dpif_port);
 netdev_close(data->netdev); /* unref and possibly close */
-hmap_remove(&port_to_netdev, &data->node);
+hmap_remove(&port_to_netdev, &data->portno_node);
+hmap_remove(&ifindex_to_port, &data->ifindex_node);
 free(data);
 ret = 0;
 }
@@ -2309,13 +2272,13 @@ netdev_ports_remove(odp_port_t port_no, const struct 
dpif_class *dpif_class)
 odp_port_t
 netdev_ifindex_to_odp_port(int ifindex)
 {
-struct ifindex_to_port_data *data;
+struct port_to_netdev_data *data;
 odp_port_t ret = 0;
 
 ovs_mutex_lock(&netdev_hmap_mutex);
-HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) {
+HMAP_FOR_EACH_WITH_HASH (data, ifindex_node, ifindex, &ifindex_to_port) {
 if (data->ifindex == ifindex) {
-ret = data->port;
+ret = data->dpif_