Date: Mon, 30 Jun 2008 18:25:31 -0700 From: Andrew Thompson <thompsa@FreeBSD.org> To: Pyun YongHyeon <pyunyh@gmail.com> Cc: freebsd-net@freebsd.org, Stefan Lambrev <stefan.lambrev@moneybookers.com> Subject: Re: if_bridge turns off checksum offload of members? Message-ID: <20080701012531.GA92392@citylink.fud.org.nz> In-Reply-To: <20080630101629.GD79537@cdnetworks.co.kr> References: <4868A34C.6030304@moneybookers.com> <20080630101629.GD79537@cdnetworks.co.kr>
next in thread | previous in thread | raw e-mail | index | archive | help
--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+--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080701012531.GA92392>