ofproto: Honor mtu_request even for internal ports.

By default Open vSwitch tries to configure internal interfaces MTU to
match the bridge minimum, overriding any attempt by the user to
configure it through standard system tools, or the database.

While this works in many simple cases (there are probably many users
that rely on this) it may create problems for more advanced use cases
(like any overlay networks).

This commit allows the user to override the default behavior by
providing an explict MTU in the mtu_request column in the Interface
table.

This means that Open vSwitch will now treat differently database MTU
requests from standard system tools MTU requests (coming from `ip link`
or `ifconfig`), but this seems the best way to remain compatible with
old users while providing a more powerful interface.

Suggested-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Acked-by: Ben Pfaff <blp@ovn.org>
Tested-by: Joe Stringer <joe@ovn.org>

FAQ.md

@@ -1065,6 +1065,33 @@ A: This is expected behavior on virtual switches.  RFC2544 tests were
 
    ovs-vsctl --no-wait set Open_vSwitch . other_config:max-idle=50000
 
+### Q: How can I configure the bridge internal interface MTU? Why does Open
+    vSwitch keep changing internal ports MTU?
+
+A: By default Open vSwitch overrides the internal interfaces (e.g. br0) MTU.
+   If you have just an internal interface (e.g. br0) and a physical interface
+   (e.g. eth0), then every change in MTU to eth0 will be reflected to br0.
+   Any manual MTU configuration using `ip` or `ifconfig` on internal interfaces
+   is going to be overridden by Open vSwitch to match the current bridge
+   minimum.
+
+   Sometimes this behavior is not desirable, for example with tunnels.  The
+   MTU of an internal interface can be explicitly set using the following
+   command:
+
+       ovs-vsctl set int br0 mtu_request=1450
+
+   After this, Open vSwitch will configure br0 MTU to 1450.  Since this
+   setting is in the database it will be persistent (compared to what
+   happens with `ip` or `ifconfig`).
+
+   The MTU configuration can be removed to restore the default behavior with
+
+       ovs-vsctl set int br0 mtu_request=[]
+
+   The mtu_request column can be used to configure MTU even for physical
+   interfaces (e.g. eth0).
+
 ## QOS
 
 ### Q: Does OVS support Quality of Service (QoS)?

NEWS

@@ -122,7 +122,7 @@ v2.6.0 - xx xxx xxxx
      SHA-1 is no longer secure and some operating systems have started to
      disable it in OpenSSL.
    - Add 'mtu_request' column to the Interface table. It can be used to
-     configure the MTU of non-internal ports.
+     configure the MTU of the ports.
 
 
 v2.5.0 - 26 Feb 2016

lib/netdev-provider.h

@@ -63,6 +63,10 @@ struct netdev {
     struct seq *reconfigure_seq;
     uint64_t last_reconfigure_seq;
 
+    /* If this is 'true', the user explicitly specified an MTU for this
+     * netdev.  Otherwise, Open vSwitch is allowed to override it. */
+    bool mtu_user_config;
+
     /* The core netdev code initializes these at netdev construction and only
      * provide read-only access to its client.  Netdev implementations may
      * modify them. */

lib/netdev.c

@@ -877,6 +877,27 @@ netdev_set_mtu(struct netdev *netdev, int mtu)
     return error;
 }
 
+/* If 'user_config' is true, the user wants to control 'netdev''s MTU and we
+ * should not override it.  If 'user_config' is false, we may adjust
+ * 'netdev''s MTU (e.g., if 'netdev' is internal). */
+void
+netdev_mtu_user_config(struct netdev *netdev, bool user_config)
+{
+    if (netdev->mtu_user_config != user_config) {
+        netdev_change_seq_changed(netdev);
+        netdev->mtu_user_config = user_config;
+    }
+}
+
+/* Returns 'true' if the user explicitly specified an MTU value for 'netdev'.
+ * Otherwise, returns 'false', in which case we are allowed to adjust the
+ * device MTU. */
+bool
+netdev_mtu_is_user_config(struct netdev *netdev)
+{
+    return netdev->mtu_user_config;
+}
+
 /* Returns the ifindex of 'netdev', if successful, as a positive number.  On
  * failure, returns a negative errno value.
  *

lib/netdev.h

@@ -133,6 +133,8 @@ const char *netdev_get_type(const struct netdev *);
 const char *netdev_get_type_from_name(const char *);
 int netdev_get_mtu(const struct netdev *, int *mtup);
 int netdev_set_mtu(struct netdev *, int mtu);
+void netdev_mtu_user_config(struct netdev *, bool);
+bool netdev_mtu_is_user_config(struct netdev *);
 int netdev_get_ifindex(const struct netdev *);
 int netdev_set_tx_multiq(struct netdev *, unsigned int n_txq);
 

ofproto/ofproto.c

@@ -175,8 +175,8 @@ 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 bool ofport_is_mtu_overridden(const struct ofproto *,
+                                     const struct ofport *);
 
 static int update_port(struct ofproto *, const char *devname);
 static int init_ports(struct ofproto *);
@@ -2416,12 +2416,12 @@ static void
 ofport_remove(struct ofport *ofport)
 {
     struct ofproto *p = ofport->ofproto;
-    bool is_internal = ofport_is_internal(p, ofport);
+    bool is_mtu_overridden = ofport_is_mtu_overridden(p, ofport);
 
     connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp,
                              OFPPR_DELETE);
     ofport_destroy(ofport, true);
-    if (!is_internal) {
+    if (!is_mtu_overridden) {
         update_mtu_ofproto(p);
     }
 }
@@ -2701,14 +2701,23 @@ init_ports(struct ofproto *p)
     return 0;
 }
 
-static inline bool
+static 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'.
+/* If 'port' is internal and if the user didn't explicitly specify an mtu
+ * through the database, we have to override it. */
+static bool
+ofport_is_mtu_overridden(const struct ofproto *p, const struct ofport *port)
+{
+    return ofport_is_internal(p, port)
+           && !netdev_mtu_is_user_config(port->netdev);
+}
+
+/* Find the minimum MTU of all non-overridden devices attached to 'p'.
  * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */
 static int
 find_min_mtu(struct ofproto *p)
@@ -2720,9 +2729,8 @@ find_min_mtu(struct ofproto *p)
         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)) {
+        /* Skip any overridden port, since that's what we're trying to set. */
+        if (ofport_is_mtu_overridden(p, ofport)) {
             continue;
         }
 
@@ -2737,8 +2745,8 @@ find_min_mtu(struct ofproto *p)
     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. */
+/* Update MTU of all overridden devices on 'p' to the minimum of the
+ * non-overridden ports in event of 'port' added or changed. */
 static void
 update_mtu(struct ofproto *p, struct ofport *port)
 {
@@ -2749,25 +2757,25 @@ update_mtu(struct ofproto *p, struct ofport *port)
         port->mtu = 0;
         return;
     }
-    if (ofport_is_internal(p, port)) {
+    if (ofport_is_mtu_overridden(p, port)) {
         if (dev_mtu > p->min_mtu) {
-           if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
-               dev_mtu = p->min_mtu;
-           }
+            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. */
+    /* For non-overridden 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;
 
@@ -2779,7 +2787,7 @@ update_mtu_ofproto(struct ofproto *p)
     HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
         struct netdev *netdev = ofport->netdev;
 
-        if (ofport_is_internal(p, ofport)) {
+        if (ofport_is_mtu_overridden(p, ofport)) {
             if (!netdev_set_mtu(netdev, p->min_mtu)) {
                 ofport->mtu = p->min_mtu;
             }

tests/ofproto-dpif.at

@@ -8892,5 +8892,20 @@ 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])
 
+# Explicitly set mtu_request on the internal interface.  This should prevent
+# the MTU from being overriden.
+AT_CHECK([ovs-vsctl set int br0 mtu_request=1700])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1700])
+
+# The new MTU on p2 should not affect br0.
+AT_CHECK([ovs-vsctl set int p2 mtu_request=1400])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p2 mtu=1400])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1700])
+
+# Remove explicit mtu_request from br0.  Now it should track the bridge
+# minimum again.
+AT_CHECK([ovs-vsctl set int br0 mtu_request=[[]]])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1400])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP

vswitchd/bridge.c

@@ -722,20 +722,20 @@ add_ofp_port(ofp_port_t port, ofp_port_t *ports, size_t *n, size_t *allocated)
 }
 
 /* Configures the MTU of 'netdev' based on the "mtu_request" column
- * in 'iface_cfg'.  'br_type' must be the datapath_type of the bridge
- * which contains 'netdev'. */
+ * in 'iface_cfg'. */
 static int
 iface_set_netdev_mtu(const struct ovsrec_interface *iface_cfg,
-                     const char *br_type, struct netdev *netdev)
+                     struct netdev *netdev)
 {
-    if (iface_cfg->n_mtu_request == 1
-        && strcmp(netdev_get_type(netdev),
-                  ofproto_port_open_type(br_type, "internal"))) {
-        /* Try to set the MTU to the requested value.  This is not done
-         * for internal interfaces, since their MTU is decided by the
-         * ofproto module, based on other ports in the bridge. */
+    if (iface_cfg->n_mtu_request == 1) {
+        /* The user explicitly asked for this MTU. */
+        netdev_mtu_user_config(netdev, true);
+        /* Try to set the MTU to the requested value. */
         return netdev_set_mtu(netdev, *iface_cfg->mtu_request);
     }
+
+    /* The user didn't explicitly asked for any MTU. */
+    netdev_mtu_user_config(netdev, false);
     return 0;
 }
 
@@ -793,7 +793,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
             goto delete;
         }
 
-        iface_set_netdev_mtu(iface->cfg, br->type, iface->netdev);
+        iface_set_netdev_mtu(iface->cfg, iface->netdev);
 
         /* If the requested OpenFlow port for 'iface' changed, and it's not
          * already the correct port, then we might want to temporarily delete
@@ -1736,7 +1736,7 @@ iface_do_create(const struct bridge *br,
         goto error;
     }
 
-    iface_set_netdev_mtu(iface_cfg, br->type, netdev);
+    iface_set_netdev_mtu(iface_cfg, netdev);
 
     *ofp_portp = iface_pick_ofport(iface_cfg);
     error = ofproto_port_add(br->ofproto, netdev, ofp_portp);

vswitchd/vswitch.xml

@@ -2388,14 +2388,16 @@
       </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">
+        <p>
+          The currently configured MTU for the interface.
+        </p>
+
         <p>
           This column will be empty for an interface that does not
           have an MTU as, for example, some kinds of tunnels do not.
@@ -2411,7 +2413,13 @@
               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>
+
+        <p>
+          If this is not set and if the interface has <code>internal</code>
+          type, Open vSwitch will change the MTU to match the minimum of the
+          other interfaces in the bridge.
         </p>
     </column>