[ovs-dev] [PATCH] ofproto: Do not override internal port MTU.
Darrell Ball
dlu998 at gmail.com
Wed Aug 31 23:28:53 UTC 2016
On Wed, Aug 31, 2016 at 2:52 PM, Daniele Di Proietto <diproiettod at 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 <diproiettod at 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 at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list