Re: if_bridge turns off checksum offload of members?

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
=== 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)
_______________________________________________
freebsd-net@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscribe@xxxxxxxxxxx"

Relevant Pages