From owner-freebsd-net@FreeBSD.ORG Tue Jul 1 01:24:10 2008 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 53730106567F for ; Tue, 1 Jul 2008 01:24:10 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: from pele.citylink.co.nz (pele.citylink.co.nz [202.8.44.226]) by mx1.freebsd.org (Postfix) with ESMTP id C97568FC1A for ; Tue, 1 Jul 2008 01:24:09 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: from localhost (localhost [127.0.0.1]) by pele.citylink.co.nz (Postfix) with ESMTP id 983CA2BE31; Tue, 1 Jul 2008 13:24:08 +1200 (NZST) X-Virus-Scanned: Debian amavisd-new at citylink.co.nz Received: from pele.citylink.co.nz ([127.0.0.1]) by localhost (pele.citylink.co.nz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZaWKx4DGUtRG; Tue, 1 Jul 2008 13:24:04 +1200 (NZST) Received: from citylink.fud.org.nz (unknown [202.8.44.45]) by pele.citylink.co.nz (Postfix) with ESMTP; Tue, 1 Jul 2008 13:24:04 +1200 (NZST) Received: by citylink.fud.org.nz (Postfix, from userid 1001) id A60DC1142C; Tue, 1 Jul 2008 13:25:31 +1200 (NZST) Date: Mon, 30 Jun 2008 18:25:31 -0700 From: Andrew Thompson To: Pyun YongHyeon Message-ID: <20080701012531.GA92392@citylink.fud.org.nz> References: <4868A34C.6030304@moneybookers.com> <20080630101629.GD79537@cdnetworks.co.kr> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="mYCpIKhGyMATD0i+" Content-Disposition: inline In-Reply-To: <20080630101629.GD79537@cdnetworks.co.kr> User-Agent: Mutt/1.5.17 (2007-11-01) Cc: freebsd-net@freebsd.org, Stefan Lambrev Subject: Re: if_bridge turns off checksum offload of members? X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Jul 2008 01:24:10 -0000 --mYCpIKhGyMATD0i+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jun 30, 2008 at 07:16:29PM +0900, Pyun YongHyeon wrote: > On Mon, Jun 30, 2008 at 12:11:40PM +0300, Stefan Lambrev wrote: > > Greetings, > > > > I just noticed, that when I add em network card to bridge the checksum > > offload is turned off. > > I even put in my rc.conf: > > ifconfig_em0="rxcsum up" > > ifconfig_em1="rxcsum up" > > but after reboot both em0 and em1 have this feature disabled. > > > > Is this expected behavior? Should I care about csum in bridge mode? > > I noticed that enabling checksum offload manually improve things little btw. > > > > AFAIK this is intended one, bridge(4) turns off Tx side checksum > offload by default. I think disabling Tx checksum offload is > required as not all members of a bridge may be able to do checksum > offload. The same is true for TSO but it seems that bridge(4) > doesn't disable it. > If all members of bridge have the same hardware capability I think > bridge(4) may not need to disable Tx side hardware assistance. I > guess bridge(4) can scan every interface capabilities in a member > and can decide what hardware assistance can be activated instead of > blindly turning off Tx side hardware assistance. This patch should do that, are you able to test it Stefan? cheers, Andrew --mYCpIKhGyMATD0i+ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="bridge_caps.diff" === if_bridge.c ================================================================== --- if_bridge.c (revision 180136) +++ if_bridge.c (local) @@ -163,9 +163,10 @@ #endif /* - * List of capabilities to mask on the member interface. + * List of capabilities to possibly mask on the member interface. */ -#define BRIDGE_IFCAPS_MASK IFCAP_TXCSUM +#define BRIDGE_IFCAPS_MASK (IFCAP_TOE|IFCAP_TSO|IFCAP_TXCSUM) +#define BRIDGE_IFCAPS_STRIP IFCAP_LRO /* * Bridge interface list entry. @@ -175,7 +176,7 @@ struct ifnet *bif_ifp; /* member if */ struct bstp_port bif_stp; /* STP state */ uint32_t bif_flags; /* member if flags */ - int bif_mutecap; /* member muted caps */ + int bif_savedcaps; /* saved capabilities */ uint32_t bif_addrmax; /* max # of addresses */ uint32_t bif_addrcnt; /* cur. # of addresses */ uint32_t bif_addrexceeded;/* # of address violations */ @@ -229,7 +230,9 @@ static void bridge_clone_destroy(struct ifnet *); static int bridge_ioctl(struct ifnet *, u_long, caddr_t); -static void bridge_mutecaps(struct bridge_iflist *, int); +static void bridge_capabilities(struct bridge_softc *); +static void bridge_set_ifcap(struct bridge_softc *, struct bridge_iflist *, + int); static void bridge_ifdetach(void *arg __unused, struct ifnet *); static void bridge_init(void *); static void bridge_dummynet(struct mbuf *, struct ifnet *); @@ -771,37 +774,52 @@ } /* - * bridge_mutecaps: + * bridge_capabilities: * * Clear or restore unwanted capabilities on the member interface */ static void -bridge_mutecaps(struct bridge_iflist *bif, int mute) +bridge_capabilities(struct bridge_softc *sc) { + struct bridge_iflist *bif; + int enabled, mask; + + mask = BRIDGE_IFCAPS_MASK; + + /* Get the set of capabilities supported */ + LIST_FOREACH(bif, &sc->sc_iflist, bif_next) { + mask &= bif->bif_ifp->if_capenable; + } + + /* Update all member interfaces to the new caps set */ + LIST_FOREACH(bif, &sc->sc_iflist, bif_next) { + enabled = bif->bif_ifp->if_capenable; + /* strip off bits and enable them again if allowed */ + enabled &= ~(BRIDGE_IFCAPS_STRIP|BRIDGE_IFCAPS_MASK); + enabled |= mask; + bridge_set_ifcap(sc, bif, enabled); + } + +} + +static void +bridge_set_ifcap(struct bridge_softc *sc, struct bridge_iflist *bif, int set) +{ struct ifnet *ifp = bif->bif_ifp; struct ifreq ifr; int error; - if (ifp->if_ioctl == NULL) - return; - bzero(&ifr, sizeof(ifr)); - ifr.ifr_reqcap = ifp->if_capenable; + ifr.ifr_reqcap = set; - if (mute) { - /* mask off and save capabilities */ - bif->bif_mutecap = ifr.ifr_reqcap & BRIDGE_IFCAPS_MASK; - if (bif->bif_mutecap != 0) - ifr.ifr_reqcap &= ~BRIDGE_IFCAPS_MASK; - } else - /* restore muted capabilities */ - ifr.ifr_reqcap |= bif->bif_mutecap; - - - if (bif->bif_mutecap != 0) { + if (ifp->if_capenable != set) { IFF_LOCKGIANT(ifp); - error = (*ifp->if_ioctl)(ifp, SIOCSIFCAP, (caddr_t)&ifr); + error = ifp->if_ioctl(ifp, SIOCSIFCAP, (caddr_t)&ifr); IFF_UNLOCKGIANT(ifp); + if (error) + if_printf(sc->sc_ifp, + "error setting interface capabilities on %s\n", + ifp->if_xname); } } @@ -868,7 +886,6 @@ * Take the interface out of promiscuous mode. */ (void) ifpromisc(ifs, 0); - bridge_mutecaps(bif, 0); break; case IFT_GIF: @@ -880,6 +897,8 @@ #endif break; } + /* reneable any interface capabilities */ + bridge_set_ifcap(sc, bif, bif->bif_savedcaps); } if (bif->bif_flags & IFBIF_STP) @@ -928,6 +947,8 @@ ifs = ifunit(req->ifbr_ifsname); if (ifs == NULL) return (ENOENT); + if (ifs->if_ioctl == NULL) /* must be supported */ + return (EINVAL); /* If it's in the span list, it can't be a member. */ LIST_FOREACH(bif, &sc->sc_spanlist, bif_next) @@ -957,6 +978,7 @@ bif->bif_ifp = ifs; bif->bif_flags = IFBIF_LEARNING | IFBIF_DISCOVER; + bif->bif_savedcaps = ifs->if_capabilities; switch (ifs->if_type) { case IFT_ETHER: @@ -967,8 +989,6 @@ error = ifpromisc(ifs, 1); if (error) goto out; - - bridge_mutecaps(bif, 1); break; case IFT_GIF: @@ -988,6 +1008,8 @@ */ LIST_INSERT_HEAD(&sc->sc_iflist, bif, bif_next); + /* Set interface capabilities to the intersection set of all members */ + bridge_capabilities(sc); out: if (error) { if (bif != NULL) --mYCpIKhGyMATD0i+--