Date: Fri, 4 Dec 2009 11:51:40 -0800 From: Pyun YongHyeon <pyunyh@gmail.com> To: Igor Sysoev <is@rambler-co.ru> Cc: freebsd-net@freebsd.org Subject: Re: hw.bge.forced_collapse Message-ID: <20091204195140.GH16491@michelle.cdnetworks.com> In-Reply-To: <20091204191114.GB76992@rambler-co.ru> References: <20091204075440.GH14822@rambler-co.ru> <20091204173243.GC16491@michelle.cdnetworks.com> <20091204191114.GB76992@rambler-co.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Fri, Dec 04, 2009 at 10:11:14PM +0300, Igor Sysoev wrote:
> On Fri, Dec 04, 2009 at 09:32:43AM -0800, Pyun YongHyeon wrote:
>
> > On Fri, Dec 04, 2009 at 10:54:40AM +0300, Igor Sysoev wrote:
> > > I saw commit introducing hw.bge.forced_collapse loader tunable.
> > > Just intresting, why it can not be a sysctl ?
> >
> > I didn't think the sysctl variable would be frequently changed
> > in runtime except debugging driver so I took simple path.
>
> I do not think it's worth to reboot server just to look how various
> values affect on bandwidth and CPU usage, expecially in production.
>
> As I understand the change is trivial:
>
> - CTLFLAG_RD
> + CTLFLAG_RW
>
> since bge_forced_collapse is used atomically.
>
I have no problem changing it to RW but that case I may have to
create actual sysctl node(e.g. dev.bge.0.forced_collapse) instead
of hw.bge.forced_collapse which may affect all bge(4) controllers
on system. Attached patch may be what you want. You can change the
value at any time.
[-- Attachment #2 --]
Index: sys/dev/bge/if_bgereg.h
===================================================================
--- sys/dev/bge/if_bgereg.h (revision 200104)
+++ sys/dev/bge/if_bgereg.h (working copy)
@@ -2647,6 +2647,7 @@
int bge_link; /* link state */
int bge_link_evt; /* pending link event */
int bge_timer;
+ int bge_forced_collapse;
struct callout bge_stat_ch;
uint32_t bge_rx_discards;
uint32_t bge_tx_discards;
Index: sys/dev/bge/if_bge.c
===================================================================
--- sys/dev/bge/if_bge.c (revision 200104)
+++ sys/dev/bge/if_bge.c (working copy)
@@ -449,6 +449,7 @@
#endif
static void bge_add_sysctls(struct bge_softc *);
static int bge_sysctl_stats(SYSCTL_HANDLER_ARGS);
+static int bge_sysctl_forced_collapse(SYSCTL_HANDLER_ARGS);
static device_method_t bge_methods[] = {
/* Device interface */
@@ -483,29 +484,12 @@
DRIVER_MODULE(miibus, bge, miibus_driver, miibus_devclass, 0, 0);
static int bge_allow_asf = 1;
-/*
- * A common design characteristic for many Broadcom client controllers
- * is that they only support a single outstanding DMA read operation
- * on the PCIe bus. This means that it will take twice as long to fetch
- * a TX frame that is split into header and payload buffers as it does
- * to fetch a single, contiguous TX frame (2 reads vs. 1 read). For
- * these controllers, coalescing buffers to reduce the number of memory
- * reads is effective way to get maximum performance(about 940Mbps).
- * Without collapsing TX buffers the maximum TCP bulk transfer
- * performance is about 850Mbps. However forcing coalescing mbufs
- * consumes a lot of CPU cycles, so leave it off by default.
- */
-static int bge_forced_collapse = 0;
TUNABLE_INT("hw.bge.allow_asf", &bge_allow_asf);
-TUNABLE_INT("hw.bge.forced_collapse", &bge_forced_collapse);
SYSCTL_NODE(_hw, OID_AUTO, bge, CTLFLAG_RD, 0, "BGE driver parameters");
SYSCTL_INT(_hw_bge, OID_AUTO, allow_asf, CTLFLAG_RD, &bge_allow_asf, 0,
"Allow ASF mode if available");
-SYSCTL_INT(_hw_bge, OID_AUTO, forced_collapse, CTLFLAG_RD, &bge_forced_collapse,
- 0, "Number of fragmented TX buffers of a frame allowed before "
- "forced collapsing");
#define SPARC64_BLADE_1500_MODEL "SUNW,Sun-Blade-1500"
#define SPARC64_BLADE_1500_PATH_BGE "/pci@1f,700000/network@2"
@@ -3933,17 +3917,17 @@
}
if ((m->m_pkthdr.csum_flags & CSUM_TSO) == 0 &&
- bge_forced_collapse > 0 && (sc->bge_flags & BGE_FLAG_PCIE) != 0 &&
- m->m_next != NULL) {
+ sc->bge_forced_collapse > 0 &&
+ (sc->bge_flags & BGE_FLAG_PCIE) != 0 && m->m_next != NULL) {
/*
* Forcedly collapse mbuf chains to overcome hardware
* limitation which only support a single outstanding
* DMA read operation.
*/
- if (bge_forced_collapse == 1)
+ if (sc->bge_forced_collapse == 1)
m = m_defrag(m, M_DONTWAIT);
else
- m = m_collapse(m, M_DONTWAIT, bge_forced_collapse);
+ m = m_collapse(m, M_DONTWAIT, sc->bge_forced_collapse);
if (m == NULL) {
m_freem(*m_head);
*m_head = NULL;
@@ -4879,6 +4863,7 @@
struct sysctl_ctx_list *ctx;
struct sysctl_oid_list *children, *schildren;
struct sysctl_oid *tree;
+ int error;
ctx = device_get_sysctl_ctx(sc->bge_dev);
children = SYSCTL_CHILDREN(device_get_sysctl_tree(sc->bge_dev));
@@ -4898,6 +4883,32 @@
#endif
+ /*
+ * A common design characteristic for many Broadcom client controllers
+ * is that they only support a single outstanding DMA read operation
+ * on the PCIe bus. This means that it will take twice as long to fetch
+ * a TX frame that is split into header and payload buffers as it does
+ * to fetch a single, contiguous TX frame (2 reads vs. 1 read). For
+ * these controllers, coalescing buffers to reduce the number of memory
+ * reads is effective way to get maximum performance(about 940Mbps).
+ * Without collapsing TX buffers the maximum TCP bulk transfer
+ * performance is about 850Mbps. However forcing coalescing mbufs
+ * consumes a lot of CPU cycles, so leave it off by default.
+ */
+ SYSCTL_ADD_PROC(ctx, children, OID_AUTO, "forced_collapse",
+ CTLTYPE_INT | CTLFLAG_RW, &sc->bge_forced_collapse, 0,
+ bge_sysctl_forced_collapse, "I", "Number of fragmented TX "
+ "buffers of a frame allowed before forced collapsing");
+ sc->bge_forced_collapse = 0;
+ error = resource_int_value(device_get_name(sc->bge_dev),
+ device_get_unit(sc->bge_dev), "forced_collapse",
+ &sc->bge_forced_collapse);
+ if (error == 0) {
+ if (sc->bge_forced_collapse < 0 ||
+ sc->bge_forced_collapse > BGE_NSEG_NEW);
+ sc->bge_forced_collapse = 0;
+ }
+
if (BGE_IS_5705_PLUS(sc))
return;
@@ -5143,6 +5154,23 @@
#endif
static int
+bge_sysctl_forced_collapse(SYSCTL_HANDLER_ARGS)
+{
+ int error, value;
+
+ if (arg1 == NULL)
+ return (EINVAL);
+ value = *(int *)arg1;
+ error = sysctl_handle_int(oidp, &value, 0, req);
+ if (error || req->newptr == NULL)
+ return (error);
+ if (value < 0 || value > BGE_NSEG_NEW)
+ return (EINVAL);
+ *(int *)arg1 = value;
+ return (0);
+}
+
+static int
bge_get_eaddr_fw(struct bge_softc *sc, uint8_t ether_addr[])
{
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091204195140.GH16491>