diff mbox

[ovs-dev,v2,2/2] ofproto: Honor mtu_request even for internal ports.

Message ID 20160902182322.77442-2-diproiettod@vmware.com
State Accepted
Headers show

Commit Message

Daniele Di Proietto Sept. 2, 2016, 6:23 p.m. UTC 
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>
---
 FAQ.md                | 27 +++++++++++++++++++++++++++
 lib/netdev-provider.h |  4 ++++
 lib/netdev.c          | 21 +++++++++++++++++++++
 lib/netdev.h          |  2 ++
 ofproto/ofproto.c     | 44 ++++++++++++++++++++++++++------------------
 tests/ofproto-dpif.at | 15 +++++++++++++++
 vswitchd/bridge.c     | 22 +++++++++++-----------
 vswitchd/vswitch.xml  | 20 ++++++++++++++------
 8 files changed, 120 insertions(+), 35 deletions(-)

Comments

Ben Pfaff Sept. 2, 2016, 9:02 p.m. UTC | #1 
On Fri, Sep 02, 2016 at 11:23:22AM -0700, Daniele Di Proietto wrote:
> 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>

Thanks for taking care of all of this.

For both:
Acked-by: Ben Pfaff <blp@ovn.org>
Joe Stringer Sept. 2, 2016, 9:43 p.m. UTC | #2 
On 2 September 2016 at 14:02, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, Sep 02, 2016 at 11:23:22AM -0700, Daniele Di Proietto wrote:
>> 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>
>
> Thanks for taking care of all of this.
>
> For both:
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks, this fixes up the failing check-{system-userspace,kernel} testsuites.

Tested-by: Joe Stringer <joe@ovn.org>
Daniele Di Proietto Sept. 2, 2016, 11:07 p.m. UTC | #3 
On 02/09/2016 14:43, "Joe Stringer" <joe@ovn.org> wrote:

>On 2 September 2016 at 14:02, Ben Pfaff <blp@ovn.org> wrote:

>> On Fri, Sep 02, 2016 at 11:23:22AM -0700, Daniele Di Proietto wrote:

>>> 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>

>>

>> Thanks for taking care of all of this.

>>

>> For both:

>> Acked-by: Ben Pfaff <blp@ovn.org>


Sorry for all the confusion and thanks for the review

>

>Thanks, this fixes up the failing check-{system-userspace,kernel} testsuites.

>

>Tested-by: Joe Stringer <joe@ovn.org>


Thanks for testing this

I updated NEWS and applied this to master and branch-2.6
diff mbox

Patch

diff --git a/FAQ.md b/FAQ.md
index 5a09d13..01e2648 100644
--- a/FAQ.md
+++ b/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)?
diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index cd04ae9..c8507a5 100644
--- a/lib/netdev-provider.h
+++ b/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. */
diff --git a/lib/netdev.c b/lib/netdev.c
index 10f2d0f..ddf2a34 100644
--- a/lib/netdev.c
+++ b/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.
  *
diff --git a/lib/netdev.h b/lib/netdev.h
index d8ec627..634c665 100644
--- a/lib/netdev.h
+++ b/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);
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index da7372d..f813c0b 100644
--- a/ofproto/ofproto.c
+++ b/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;
             }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index d7705da..0295a99 100644
--- a/tests/ofproto-dpif.at
+++ b/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
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a7a12ec..61cb966 100644
--- a/vswitchd/bridge.c
+++ b/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);
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 69b5592..f64c18a 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -2388,15 +2388,17 @@ 
       </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.
         </p>
@@ -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>