[libvirt] [PATCH] bridge: set MTU of tap iface to the same value of the bridge
Hi, This is a follow-up to the RFC patch sent by Chris Wright, at: https://www.redhat.com/archives/libvir-list/2008-November/msg00225.html With this patch, instead of hardcoding the MTU to an high value, we set the MTU of the tap interface to the same MTU value set on the bridge interface. The current code uses the default tap MTU value and lowers the bridge interface MTU without any need, killing performance. I will send another patch later for allowing the tap MTU to be configured on the XML, but the behavior added by this patch will be kept as the default. The default behavior should be enough for most use cases where a larger MTU is wanted for the bridged interfaces. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- src/bridge.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 98 insertions(+), 0 deletions(-) diff --git a/src/bridge.c b/src/bridge.c index 4090163..60334b6 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -276,6 +276,98 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, #endif /** + * ifGetMtu + * @ctl: bridge control pointer + * @ifname: interface name get MTU for + * + * This function gets the @mtu value set for a given interface @ifname. + * + * Returns the MTU value in case of success. + * On error, returns -1 and sets errno accordingly + */ +static int ifGetMtu(brControl *ctl, const char *ifname) +{ + struct ifreq ifr; + int len; + + if (!ctl || !ifname) { + errno = EINVAL; + return -1; + } + + if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) { + errno = EINVAL; + return -1; + } + + memset(&ifr, 0, sizeof(struct ifreq)); + + strncpy(ifr.ifr_name, ifname, len); + ifr.ifr_name[len] = '\0'; + + if (ioctl(ctl->fd, SIOCGIFMTU, &ifr)) + return -1; + + return ifr.ifr_mtu; + +} + +/** + * ifSetMtu: + * @ctl: bridge control pointer + * @ifname: interface name to set MTU for + * @mtu: MTU value + * + * This function sets the @mtu for a given interface @ifname. Typically + * used on a tap device to set up for Jumbo Frames. + * + * Returns 0 in case of success or an errno code in case of failure. + */ +static int ifSetMtu(brControl *ctl, const char *ifname, int mtu) +{ + struct ifreq ifr; + int len; + + if (!ctl || !ifname) + return EINVAL; + + if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) + return EINVAL; + + memset(&ifr, 0, sizeof(struct ifreq)); + + strncpy(ifr.ifr_name, ifname, len); + ifr.ifr_name[len] = '\0'; + ifr.ifr_mtu = mtu; + + return ioctl(ctl->fd, SIOCSIFMTU, &ifr) == 0 ? 0 : errno; +} + +/** + * brSetInterfaceMtu + * @ctl: bridge control pointer + * @bridge: name of the bridge interface + * @ifname: name of the interface whose MTU we want to set + * + * Sets the interface mtu to the same MTU of the bridge + * + * Returns 0 in case of success or an errno code in case of failure. + */ +static int brSetInterfaceMtu(brControl *ctl, + const char *bridge, + const char *ifname) +{ + int mtu = ifGetMtu(ctl, bridge); + + fprintf(stderr, "mtu: %d\n", mtu); + + if (mtu < 0) + return errno; + + return ifSetMtu(ctl, ifname, mtu); +} + +/** * brAddTap: * @ctl: bridge control pointer * @bridge: the bridge name @@ -334,6 +426,12 @@ brAddTap(brControl *ctl, } if (ioctl(fd, TUNSETIFF, &try) == 0) { + /* We need to set the interface MTU before adding it + * to the bridge, because the bridge will have its + * MTU adjusted automatically when we add the new interface. + */ + if ((errno = brSetInterfaceMtu(ctl, bridge, try.ifr_name))) + goto error; if ((errno = brAddInterface(ctl, bridge, try.ifr_name))) goto error; if ((errno = brSetInterfaceUp(ctl, try.ifr_name, 1))) -- 1.5.5.GIT
On Tue, Dec 02, 2008 at 07:08:37PM -0200, Eduardo Habkost wrote:
Hi,
This is a follow-up to the RFC patch sent by Chris Wright, at: https://www.redhat.com/archives/libvir-list/2008-November/msg00225.html
With this patch, instead of hardcoding the MTU to an high value, we set the MTU of the tap interface to the same MTU value set on the bridge interface. The current code uses the default tap MTU value and lowers the bridge interface MTU without any need, killing performance.
ACK, this looks like a reasonable approach to - the initial physical NIC attached to the bridge will set the preferred MTU, so we're just following that now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
* Eduardo Habkost (ehabkost@redhat.com) wrote:
+static int brSetInterfaceMtu(brControl *ctl, + const char *bridge, + const char *ifname) +{ + int mtu = ifGetMtu(ctl, bridge); + + fprintf(stderr, "mtu: %d\n", mtu);
Is that just a bit of leftover debugging? Other than that, looks good to me. ACK
On Tue, Dec 02, 2008 at 10:15:51PM -0800, Chris Wright wrote:
* Eduardo Habkost (ehabkost@redhat.com) wrote:
+static int brSetInterfaceMtu(brControl *ctl, + const char *bridge, + const char *ifname) +{ + int mtu = ifGetMtu(ctl, bridge); + + fprintf(stderr, "mtu: %d\n", mtu);
Is that just a bit of leftover debugging?
Oops. Updated patch below. --- From: Eduardo Habkost <ehabkost@redhat.com> Date: Tue, 2 Dec 2008 16:37:00 -0200 Subject: [PATCH] bridge: set MTU of tap iface to the same value of the bridge This is a follow-up to the RFC patch sent by Chris Wright, at: https://www.redhat.com/archives/libvir-list/2008-November/msg00225.html With this patch, instead of hardcoding the MTU to an high value, we set the MTU of the tap interface to the same MTU value set on the bridge interface. The current code uses the default tap MTU value and lowers the bridge interface MTU without any need, killing performance. I will send another patch later for allowing the tap MTU to be configured on the XML, but the behavior added by this patch will be kept as the default. The default behavior should be enough for most use cases where a larger MTU is wanted for the bridged interfaces. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- src/bridge.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 96 insertions(+), 0 deletions(-) diff --git a/src/bridge.c b/src/bridge.c index 4090163..13d81bc 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -276,6 +276,96 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, #endif /** + * ifGetMtu + * @ctl: bridge control pointer + * @ifname: interface name get MTU for + * + * This function gets the @mtu value set for a given interface @ifname. + * + * Returns the MTU value in case of success. + * On error, returns -1 and sets errno accordingly + */ +static int ifGetMtu(brControl *ctl, const char *ifname) +{ + struct ifreq ifr; + int len; + + if (!ctl || !ifname) { + errno = EINVAL; + return -1; + } + + if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) { + errno = EINVAL; + return -1; + } + + memset(&ifr, 0, sizeof(struct ifreq)); + + strncpy(ifr.ifr_name, ifname, len); + ifr.ifr_name[len] = '\0'; + + if (ioctl(ctl->fd, SIOCGIFMTU, &ifr)) + return -1; + + return ifr.ifr_mtu; + +} + +/** + * ifSetMtu: + * @ctl: bridge control pointer + * @ifname: interface name to set MTU for + * @mtu: MTU value + * + * This function sets the @mtu for a given interface @ifname. Typically + * used on a tap device to set up for Jumbo Frames. + * + * Returns 0 in case of success or an errno code in case of failure. + */ +static int ifSetMtu(brControl *ctl, const char *ifname, int mtu) +{ + struct ifreq ifr; + int len; + + if (!ctl || !ifname) + return EINVAL; + + if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) + return EINVAL; + + memset(&ifr, 0, sizeof(struct ifreq)); + + strncpy(ifr.ifr_name, ifname, len); + ifr.ifr_name[len] = '\0'; + ifr.ifr_mtu = mtu; + + return ioctl(ctl->fd, SIOCSIFMTU, &ifr) == 0 ? 0 : errno; +} + +/** + * brSetInterfaceMtu + * @ctl: bridge control pointer + * @bridge: name of the bridge interface + * @ifname: name of the interface whose MTU we want to set + * + * Sets the interface mtu to the same MTU of the bridge + * + * Returns 0 in case of success or an errno code in case of failure. + */ +static int brSetInterfaceMtu(brControl *ctl, + const char *bridge, + const char *ifname) +{ + int mtu = ifGetMtu(ctl, bridge); + + if (mtu < 0) + return errno; + + return ifSetMtu(ctl, ifname, mtu); +} + +/** * brAddTap: * @ctl: bridge control pointer * @bridge: the bridge name @@ -334,6 +424,12 @@ brAddTap(brControl *ctl, } if (ioctl(fd, TUNSETIFF, &try) == 0) { + /* We need to set the interface MTU before adding it + * to the bridge, because the bridge will have its + * MTU adjusted automatically when we add the new interface. + */ + if ((errno = brSetInterfaceMtu(ctl, bridge, try.ifr_name))) + goto error; if ((errno = brAddInterface(ctl, bridge, try.ifr_name))) goto error; if ((errno = brSetInterfaceUp(ctl, try.ifr_name, 1))) -- 1.5.5.GIT -- Eduardo
On Wed, 2008-12-03 at 11:14 -0200, Eduardo Habkost wrote:
+ * Returns the MTU value in case of success. + * On error, returns -1 and sets errno accordingly + */ +static int ifGetMtu(brControl *ctl, const char *ifname)
You could make this return -errno on error. No big deal, though. Cheers, Mark.
On Wed, Dec 03, 2008 at 02:14:24PM +0000, Mark McLoughlin wrote:
On Wed, 2008-12-03 at 11:14 -0200, Eduardo Habkost wrote:
+ * Returns the MTU value in case of success. + * On error, returns -1 and sets errno accordingly + */ +static int ifGetMtu(brControl *ctl, const char *ifname)
You could make this return -errno on error. No big deal, though.
I was going to do that, but returning negative errno values didn't seem to an usual way of returning errors on libvirt. -- Eduardo
On Wed, Dec 03, 2008 at 11:14:28AM -0200, Eduardo Habkost wrote:
On Tue, Dec 02, 2008 at 10:15:51PM -0800, Chris Wright wrote:
* Eduardo Habkost (ehabkost@redhat.com) wrote:
+static int brSetInterfaceMtu(brControl *ctl, + const char *bridge, + const char *ifname) +{ + int mtu = ifGetMtu(ctl, bridge); + + fprintf(stderr, "mtu: %d\n", mtu);
Is that just a bit of leftover debugging?
Oops. Updated patch below.
Okidoc, I commited it, I think the errno passing of the potential errors is sufficient at this point, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, 2008-12-02 at 19:08 -0200, Eduardo Habkost wrote:
Hi,
This is a follow-up to the RFC patch sent by Chris Wright, at: https://www.redhat.com/archives/libvir-list/2008-November/msg00225.html
With this patch, instead of hardcoding the MTU to an high value, we set the MTU of the tap interface to the same MTU value set on the bridge interface. The current code uses the default tap MTU value and lowers the bridge interface MTU without any need, killing performance.
I will send another patch later for allowing the tap MTU to be configured on the XML, but the behavior added by this patch will be kept as the default. The default behavior should be enough for most use cases where a larger MTU is wanted for the bridged interfaces.
Interesting twist :-) The bridge MTU is limited to the lowest MTU of its constituents. The tap MTU is pretty irrelevant when you connect it to a bridge, since no host tx packets will originate tap device ... so the only purpose in raising the tap MTU is raising the upper limit of the bridge MTU. So basically this patch is trying to take the tap MTU out of the equation when the tap device is enslaved to a bridge. However, consider this sequence: 1) Bridge br0 exists with a single interface (eth0) enslaved and the MTU of both is 1500 2) Start guest foo, and vnet0 is enslaved to br0 with an automatic MTU of 1500 3) User tries to up the MTU of eth0 to 9000, but is prevented from doing so because the bridge MTU is constrained by the MTU of vnet0 But it would work fine if you had increased the eth0 MTU *before* starting the guest. Does it make sense to automatically set the tap MTU to a very large value so as to ensure that the bridge MTU is only constrained by the MTU of the physical device? Well, no it doesn't because in the case of a virtual network there is no physical MTU so the bridge would end up with a large MTU and there's no guarantee qemu or the host can handle that. Conclusions from above meandering: 1) Your patch is a sensible default; not perfect, but a very good compromise 2) The next thing we need is a patch to allow changing setting the MTU for the bridge in a virtual network e.g. <network> <name>foo</name> <bridge name='virbr1'/> </network> 3) I don't think having a way in libvirt to configure the MTU of an enslaved TAP device is interesting given both of the above 4) I've added a tiny hint to: http://wiki.libvirt.org/page/Networking about configuring a physical device's MTU when setting it up a shared physical interface. HTH, Mark.
On Wed, 2008-12-03 at 14:09 +0000, Mark McLoughlin wrote:
Conclusions from above meandering:
1) Your patch is a sensible default; not perfect, but a very good compromise
For DV: J'adore cette patch. C'est magnifique. S'il vous plaît s'appliquer. Merci, Mark.
participants (5)
-
Chris Wright -
Daniel P. Berrange -
Daniel Veillard -
Eduardo Habkost -
Mark McLoughlin