Message ID | CAE5AaEyqcPJJrcfcXeWjNAf2VGJQfLNwGAd47hFiPTon+PhO=w@mail.gmail.com |
---|---|
State | Accepted |
Headers | show |
Thanks for the patch! I've added your name to AUTHORS and applied this to master 2016-05-25 8:03 GMT-07:00 wisd0me <ak47izatool@gmail.com>: > When we're adding the port into ovs bridge, its mtu is updated to the > minimal > mtu of the included port. But when the port is getting removed, no such > update > is performed, which leads to bug. For example, when the port with minimal > mtu > is removed, bridge's mtu must adapt to new value, but it won't happen. > How to reproduce the problem: > > $ ovs-vsctl add-br testing > $ ip link add name gretap11 type gretap local 10.0.0.1 remote 10.0.0.100 > $ ip link add name gretap12 type gretap local 10.0.0.1 remote 10.0.0.200 > $ ip link set dev gretap12 mtu 1600 > $ ovs-vsctl add-port testing gretap11 > $ ovs-vsctl add-port testing gretap12 > $ ip a sh testing > 16: testing: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN > group default qlen 1 > link/ether 7a:42:95:00:96:40 brd ff:ff:ff:ff:ff:ff > > $ ovs-vsctl del-port gretap11 > $ ip a sh testing > 16: testing: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN > group default qlen 1 > link/ether 7a:42:95:00:96:40 brd ff:ff:ff:ff:ff:ff > > $## as we can see here, 'testing' bridge mtu is stuck, while it must > adapt to new '1600' value, > $## cause there is only one port 'gretap12' left, and it's mtu is '1600': > > $ ip a sh gretap12 > 19: gretap12@NONE: <BROADCAST,MULTICAST> mtu 1600 qdisc noop master > ovs-system state DOWN group default qlen 1000 > link/ether b2:c6:1d:9f:be:0d brd ff:ff:ff:ff:ff:ff > > My commit fixes this problem - mtu update is performed on port removal as > well. > > Signed-off-by: wisd0me <ak47izatool@gmail.com> > --- > ofproto/ofproto.c | 36 +++++++++++++++++++++++++++++------- > 1 file changed, 29 insertions(+), 7 deletions(-) > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index ae527a4..9aff687 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -219,6 +219,7 @@ static void learned_cookies_flush(struct ofproto > *, struct ovs_list *dead_cookie > /* ofport. */ > static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex); > static void ofport_destroy(struct ofport *, bool del); > +static inline bool ofport_is_internal(const struct ofport *); > > static int update_port(struct ofproto *, const char *devname); > static int init_ports(struct ofproto *); > @@ -320,6 +321,7 @@ static uint64_t pick_datapath_id(const struct ofproto > *); > static uint64_t pick_fallback_dpid(void); > static void ofproto_destroy__(struct ofproto *); > static void update_mtu(struct ofproto *, struct ofport *); > +static void update_mtu_ofproto(struct ofproto *); > static void meter_delete(struct ofproto *, uint32_t first, uint32_t last); > static void meter_insert_rule(struct rule *); > > @@ -2386,9 +2388,15 @@ error: > static void > ofport_remove(struct ofport *ofport) > { > + struct ofproto *p = ofport->ofproto; > + bool is_internal = ofport_is_internal(ofport); > + > connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp, > OFPPR_DELETE); > ofport_destroy(ofport, true); > + if (!is_internal) { > + update_mtu_ofproto(p); > + } > } > > /* If 'ofproto' contains an ofport named 'name', removes it from > 'ofproto' and > @@ -2666,6 +2674,12 @@ init_ports(struct ofproto *p) > return 0; > } > > +static inline bool > +ofport_is_internal(const struct ofport *port) > +{ > + return strcmp(netdev_get_type(port->netdev), "internal") == 0; > +} > + > /* Find the minimum MTU of all non-datapath devices attached to 'p'. > * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */ > static int > @@ -2680,7 +2694,7 @@ find_min_mtu(struct ofproto *p) > > /* Skip any internal ports, since that's what we're trying to > * set. */ > - if (!strcmp(netdev_get_type(netdev), "internal")) { > + if (ofport_is_internal(ofport)) { > continue; > } > > @@ -2700,15 +2714,14 @@ find_min_mtu(struct ofproto *p) > static void > update_mtu(struct ofproto *p, struct ofport *port) > { > - struct ofport *ofport; > struct netdev *netdev = port->netdev; > - int dev_mtu, old_min; > + int dev_mtu; > > if (netdev_get_mtu(netdev, &dev_mtu)) { > port->mtu = 0; > return; > } > - if (!strcmp(netdev_get_type(port->netdev), "internal")) { > + if (ofport_is_internal(port)) { > if (dev_mtu > p->min_mtu) { > if (!netdev_set_mtu(port->netdev, p->min_mtu)) { > dev_mtu = p->min_mtu; > @@ -2718,9 +2731,18 @@ update_mtu(struct ofproto *p, struct ofport *port) > return; > } > > - /* For non-internal port find new min mtu. */ > - old_min = p->min_mtu; > port->mtu = dev_mtu; > + /* For non-internal port find new min mtu. */ > + update_mtu_ofproto(p); > +} > + > +static void > +update_mtu_ofproto(struct ofproto *p) > +{ > + /* For non-internal port find new min mtu. */ > + struct ofport *ofport; > + int old_min = p->min_mtu; > + > p->min_mtu = find_min_mtu(p); > if (p->min_mtu == old_min) { > return; > @@ -2729,7 +2751,7 @@ update_mtu(struct ofproto *p, struct ofport *port) > HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { > struct netdev *netdev = ofport->netdev; > > - if (!strcmp(netdev_get_type(netdev), "internal")) { > + if (ofport_is_internal(ofport)) { > if (!netdev_set_mtu(netdev, p->min_mtu)) { > ofport->mtu = p->min_mtu; > } > -- > 2.7.3 > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index ae527a4..9aff687 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -219,6 +219,7 @@ static void learned_cookies_flush(struct ofproto *, struct ovs_list *dead_cookie /* ofport. */ static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex); static void ofport_destroy(struct ofport *, bool del); +static inline bool ofport_is_internal(const struct ofport *); static int update_port(struct ofproto *, const char *devname);
When we're adding the port into ovs bridge, its mtu is updated to the minimal mtu of the included port. But when the port is getting removed, no such update is performed, which leads to bug. For example, when the port with minimal mtu is removed, bridge's mtu must adapt to new value, but it won't happen. How to reproduce the problem: $ ovs-vsctl add-br testing $ ip link add name gretap11 type gretap local 10.0.0.1 remote 10.0.0.100 $ ip link add name gretap12 type gretap local 10.0.0.1 remote 10.0.0.200 $ ip link set dev gretap12 mtu 1600 $ ovs-vsctl add-port testing gretap11 $ ovs-vsctl add-port testing gretap12 $ ip a sh testing 16: testing: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN group default qlen 1 link/ether 7a:42:95:00:96:40 brd ff:ff:ff:ff:ff:ff $ ovs-vsctl del-port gretap11 $ ip a sh testing 16: testing: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN group default qlen 1 link/ether 7a:42:95:00:96:40 brd ff:ff:ff:ff:ff:ff $## as we can see here, 'testing' bridge mtu is stuck, while it must adapt to new '1600' value, $## cause there is only one port 'gretap12' left, and it's mtu is '1600': $ ip a sh gretap12 19: gretap12@NONE: <BROADCAST,MULTICAST> mtu 1600 qdisc noop master ovs-system state DOWN group default qlen 1000 link/ether b2:c6:1d:9f:be:0d brd ff:ff:ff:ff:ff:ff My commit fixes this problem - mtu update is performed on port removal as well. Signed-off-by: wisd0me <ak47izatool@gmail.com> --- ofproto/ofproto.c | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) static int init_ports(struct ofproto *); @@ -320,6 +321,7 @@ static uint64_t pick_datapath_id(const struct ofproto *); static uint64_t pick_fallback_dpid(void); static void ofproto_destroy__(struct ofproto *); static void update_mtu(struct ofproto *, struct ofport *); +static void update_mtu_ofproto(struct ofproto *); static void meter_delete(struct ofproto *, uint32_t first, uint32_t last); static void meter_insert_rule(struct rule *); @@ -2386,9 +2388,15 @@ error: static void ofport_remove(struct ofport *ofport) { + struct ofproto *p = ofport->ofproto; + bool is_internal = ofport_is_internal(ofport); + connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp, OFPPR_DELETE); ofport_destroy(ofport, true); + if (!is_internal) { + update_mtu_ofproto(p); + } } /* If 'ofproto' contains an ofport named 'name', removes it from 'ofproto' and @@ -2666,6 +2674,12 @@ init_ports(struct ofproto *p) return 0; } +static inline bool +ofport_is_internal(const struct ofport *port) +{ + return strcmp(netdev_get_type(port->netdev), "internal") == 0; +} + /* Find the minimum MTU of all non-datapath devices attached to 'p'. * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */ static int @@ -2680,7 +2694,7 @@ find_min_mtu(struct ofproto *p) /* Skip any internal ports, since that's what we're trying to * set. */ - if (!strcmp(netdev_get_type(netdev), "internal")) { + if (ofport_is_internal(ofport)) { continue; } @@ -2700,15 +2714,14 @@ find_min_mtu(struct ofproto *p) static void update_mtu(struct ofproto *p, struct ofport *port) { - struct ofport *ofport; struct netdev *netdev = port->netdev; - int dev_mtu, old_min; + int dev_mtu; if (netdev_get_mtu(netdev, &dev_mtu)) { port->mtu = 0; return; } - if (!strcmp(netdev_get_type(port->netdev), "internal")) { + if (ofport_is_internal(port)) { if (dev_mtu > p->min_mtu) { if (!netdev_set_mtu(port->netdev, p->min_mtu)) { dev_mtu = p->min_mtu; @@ -2718,9 +2731,18 @@ update_mtu(struct ofproto *p, struct ofport *port) return; } - /* For non-internal port find new min mtu. */ - old_min = p->min_mtu; port->mtu = dev_mtu; + /* For non-internal port find new min mtu. */ + update_mtu_ofproto(p); +} + +static void +update_mtu_ofproto(struct ofproto *p) +{ + /* For non-internal port find new min mtu. */ + struct ofport *ofport; + int old_min = p->min_mtu; + p->min_mtu = find_min_mtu(p); if (p->min_mtu == old_min) { return; @@ -2729,7 +2751,7 @@ update_mtu(struct ofproto *p, struct ofport *port) HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { struct netdev *netdev = ofport->netdev; - if (!strcmp(netdev_get_type(netdev), "internal")) { + if (ofport_is_internal(ofport)) { if (!netdev_set_mtu(netdev, p->min_mtu)) { ofport->mtu = p->min_mtu; } -- 2.7.3