On Wed, Aug 31, 2016 at 2:52 PM, Daniele Di Proietto <diproiet...@vmware.com > wrote:
> Open vSwitch controls the MTU of internal ports and sets it to the > minimum of physical ports MTU on the same bridge. > > Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") > made this more consistent. Now the MTU is always set, even when the > bridge doesn't have any physical ports (e.g. when it has only an > internal device and a tunnel). > > This latest change broke the system testsuite. Some tests need to > lower internal devices MTU because they use tunnels and they want to > work with a 1500 bytes underlay. > > There are other valid usecases where the user/controller needs to > configure the internal devices MTU (like mpls push actions, double vlans > or any overlay) > and there's no way for Open vSwitch to know what the > appropriate MTU should be. > > This is not a review. But, IMO, the above statement is correct. > Since in the general case Open vSwitch is not able to configure a > reasonable MTU for internal devices, this commit removes the feature > entirely. > > Now the user/controller is entirely responsible for configuring the MTU > of internal ports. Other hybrid solutions are possible (like not > touching the internal interfaces MTU, unless there's a physical device), > but they make the current MTU dependent on the previous state of the > system (if there was at some point a physical device the MTU would be > touched, but it wouldn't be possible to restore it). > > This change breaks compatibility with previous versions on Open vSwitch. > > Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com> > --- > I realize that it's not nice to introduce a backwards incompatible > change, especially so late in the release. I considered other possible > solutions, but they all introduce undesirable behavior. If it's not > acceptable to break compatibility, I can get away with alternative > approaches. > --- > NEWS | 4 ++ > debian/changelog | 4 ++ > ofproto/ofproto-provider.h | 2 - > ofproto/ofproto.c | 101 ------------------------------ > --------------- > tests/ofproto-dpif.at | 16 +------ > vswitchd/vswitch.xml | 10 ++--- > 6 files changed, 13 insertions(+), 124 deletions(-) > > diff --git a/NEWS b/NEWS > index 1439151..590a7b9 100644 > --- a/NEWS > +++ b/NEWS > @@ -123,6 +123,10 @@ v2.6.0 - xx xxx xxxx > disable it in OpenSSL. > - Add 'mtu_request' column to the Interface table. It can be used to > configure the MTU of non-internal ports. > + - Open vSwitch no longer automatically configures the internal > interfaces > + MTU to match the rest of the bridge. Please use external tools (or > + better, the 'mtu_request' column) to appropriately configure the MTU > on > + internal ports. > > > v2.5.0 - 26 Feb 2016 > diff --git a/debian/changelog b/debian/changelog > index d73e636..9958ef9 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -117,6 +117,10 @@ openvswitch (2.6.0-1) unstable; urgency=low > disable it in OpenSSL. > - Add 'mtu_request' column to the Interface table. It can be used to > configure the MTU of non-internal ports. > + - Open vSwitch no longer automatically configures the internal > interfaces > + MTU to match the rest of the bridge. Please use external tools (or > + better, the 'mtu_request' column) to appropriately configure the MTU > on > + internal ports. > > -- Open vSwitch team <dev@openvswitch.org> Mon, 15 Aug 2016 19:53:13 > -0700 > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 7f7813e..9f2c408 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -115,8 +115,6 @@ struct ofproto { > /* OpenFlow connections. */ > struct connmgr *connmgr; > > - int min_mtu; /* Current MTU of non-internal ports. > */ > - > /* Groups. */ > struct cmap groups; /* Contains "struct ofgroup"s. */ > uint32_t n_groups[4] OVS_GUARDED; /* # of existing groups of each > type. */ > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 9d62b72..9fc87de 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -175,8 +175,6 @@ 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 ofproto *, > - const struct ofport *); > > static int update_port(struct ofproto *, const char *devname); > static int init_ports(struct ofproto *); > @@ -273,16 +271,12 @@ static void calc_duration(long long int start, long > long int now, > 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 *); > > /* unixctl. */ > static void ofproto_unixctl_init(void); > > -static int find_min_mtu(struct ofproto *p); > - > /* All registered ofproto classes, in probe order. */ > static const struct ofproto_class **ofproto_classes; > static size_t n_ofproto_classes; > @@ -516,7 +510,6 @@ ofproto_create(const char *datapath_name, const char > *datapath_type, > hmap_init(&ofproto->learned_cookies); > ovs_list_init(&ofproto->expirable); > ofproto->connmgr = connmgr_create(ofproto, datapath_name, > datapath_name); > - ofproto->min_mtu = find_min_mtu(ofproto); > cmap_init(&ofproto->groups); > ovs_mutex_unlock(&ofproto_mutex); > ofproto->ogf.types = 0xf; > @@ -2392,8 +2385,6 @@ ofport_install(struct ofproto *p, > hash_ofp_port(ofport->ofp_port)); > shash_add(&p->port_by_name, netdev_name, ofport); > > - update_mtu(p, ofport); > - > /* Let the ofproto_class initialize its private data. */ > error = p->ofproto_class->port_construct(ofport); > if (error) { > @@ -2417,15 +2408,9 @@ error: > static void > ofport_remove(struct ofport *ofport) > { > - struct ofproto *p = ofport->ofproto; > - bool is_internal = ofport_is_internal(p, 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 > @@ -2621,8 +2606,6 @@ update_port(struct ofproto *ofproto, const char > *name) > ofport_modified(port, &pp); > } > > - update_mtu(ofproto, port); > - > /* Install the newly opened netdev in case it has changed. > * Don't close the old netdev yet in case port_modified has to > * remove a retained reference to it.*/ > @@ -2702,90 +2685,6 @@ init_ports(struct ofproto *p) > > return 0; > } > - > -static inline bool > -ofport_is_internal(const struct ofproto *p, const struct ofport *port) > -{ > - return !strcmp(netdev_get_type(port->netdev), > - ofproto_port_open_type(p->type, "internal")); > -} > - > -/* Find the minimum MTU of all non-datapath devices attached to 'p'. > - * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */ > -static int > -find_min_mtu(struct ofproto *p) > -{ > - struct ofport *ofport; > - int mtu = 0; > - > - HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { > - struct netdev *netdev = ofport->netdev; > - int dev_mtu; > - > - /* Skip any internal ports, since that's what we're trying to > - * set. */ > - if (ofport_is_internal(p, ofport)) { > - continue; > - } > - > - if (netdev_get_mtu(netdev, &dev_mtu)) { > - continue; > - } > - if (!mtu || dev_mtu < mtu) { > - mtu = dev_mtu; > - } > - } > - > - return mtu ? mtu: ETH_PAYLOAD_MAX; > -} > - > -/* Update MTU of all datapath devices on 'p' to the minimum of the > - * non-datapath ports in event of 'port' added or changed. */ > -static void > -update_mtu(struct ofproto *p, struct ofport *port) > -{ > - struct netdev *netdev = port->netdev; > - int dev_mtu; > - > - if (netdev_get_mtu(netdev, &dev_mtu)) { > - port->mtu = 0; > - return; > - } > - if (ofport_is_internal(p, port)) { > - if (!netdev_set_mtu(port->netdev, p->min_mtu)) { > - dev_mtu = p->min_mtu; > - } > - port->mtu = dev_mtu; > - return; > - } > - > - 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; > - } > - > - HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { > - struct netdev *netdev = ofport->netdev; > - > - if (ofport_is_internal(p, ofport)) { > - if (!netdev_set_mtu(netdev, p->min_mtu)) { > - ofport->mtu = p->min_mtu; > - } > - } > - } > -} > > static void > ofproto_rule_destroy__(struct rule *rule) > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 8e5fde6..250d99a 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -8870,10 +8870,7 @@ AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], > [dnl > > add_of_ports br0 1 > > -# Check that initial MTU is 1500 for 'br0' and 'p1'. > -AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], [dnl > -1500 > -]) > +# Check that initial MTU is 1500 for 'p1'. > AT_CHECK([ovs-vsctl get Interface p1 mtu], [0], [dnl > 1500 > ]) > @@ -8883,23 +8880,12 @@ AT_CHECK([ovs-vsctl set Interface p1 > mtu_request=1600]) > > # Check that the new MTU is applied > AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p1 mtu=1600]) > -# The internal port 'br0' should have the same MTU value as p1, becase > it's > -# the new bridge minimum. > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600]) > > AT_CHECK([ovs-vsctl del-port br0 p1]) > > -# When 'p1' is deleted, the internal port should return to the default MTU > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1500]) > - > # New port with 'mtu_request' in the same transaction. > AT_CHECK([ovs-vsctl add-port br0 p2 -- set int p2 type=dummy > mtu_request=1600]) > AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p2 mtu=1600]) > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600]) > - > -# New internal port. The MTU should be updated even for a newly added > port. > -AT_CHECK([ovs-vsctl add-port br0 int1 -- set int int1 type=internal]) > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface int1 mtu=1600]) > > OVS_VSWITCHD_STOP > AT_CLEANUP > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 69b5592..684df55 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -2388,11 +2388,9 @@ > </p> > > <p> > - A client may change a non-internal interface MTU by filling in > - <ref column="mtu_request"/>. Internal interfaces MTU, instead, > is set > - by Open vSwitch to the minimum of non-internal interfaces MTU in > the > - bridge. In any case, Open vSwitch then reports in <ref > column="mtu"/> > - the currently configured value. > + A client may change an interface MTU by filling in > + <ref column="mtu_request"/>. Open vSwitch then reports in > + <ref column="mtu"/> the currently configured value. > </p> > > <column name="mtu"> > @@ -2411,7 +2409,7 @@ > type='{"type": "integer", "minInteger": 1}'> > <p> > Requested MTU (Maximum Transmission Unit) for the interface. A > client > - can fill this column to change the MTU of a non-internal > interface. > + can fill this column to change the MTU of an interface. > </p> > </column> > > -- > 2.9.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev