Date: Sat, 22 Mar 2014 14:16:24 +0400 From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru> To: Gleb Smirnoff <glebius@FreeBSD.org> Cc: net@FreeBSD.org Subject: Re: [PATCH 2/6] sfxge: limit software Tx queue size Message-ID: <532D62F8.4080103@oktetlabs.ru> In-Reply-To: <20140318132440.GG1499@FreeBSD.org> References: <532817F5.8010505@oktetlabs.ru> <20140318132440.GG1499@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------080407050309040104050306
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Gleb,
On 03/18/2014 05:24 PM, Gleb Smirnoff wrote:
> Andrew,
>
> On Tue, Mar 18, 2014 at 01:55:01PM +0400, Andrew Rybchenko wrote:
> A> sfxge: limit software Tx queue size
> A>
> A> Previous implementation limits put queue size only (when Tx lock can't
> A> be acquired),
> A> but get queue may grow unboundedly which results in mbuf pools
> A> exhaustion and
> A> latency growth.
> A>
> A> Submitted-by: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> A> Sponsored by: Solarflare Communications, Inc.
>
> The interaction between sfxge_tx_qdpl_put() and sfxge_tx_packet_add()
> is quite complex and I couldn't resist from suggesting you to
> simplify the code.
>
> Can you please look into attached patch?
>
> - Inline sfxge_tx_qdpl_put() into sfxge_tx_packet_add().
> - Simplify the 'locked' logic.
> - Add your PATCH 1/6, the mbuf leak fix.
> - Add your PATCH 2/6, the SFXGE_TX_DPL_GET_PKT_LIMIT_DEFAULT check.
I don't like "locked" flag passed to qdpl_put() function as well.
However, I prefer to keep patches granular and avoid mixing of different
changes in single patch. If the initial patch is OK, please, submit it
to repository. Then, I'll rebase your patch, discuss it locally and come
back to you.
BTW, I see that many drivers use drbr for software Tx queue. What do you
think, would it be beneficial to use it instead of the list implemented
here?
Please, find initial patch with few minor fixes (TAB after #define and
@->" at " in suggested commit message) attached.
Thanks a lot,
Andrew.
--------------080407050309040104050306
Content-Type: text/x-patch;
name="2-sfxge-limit.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="2-sfxge-limit.patch"
sfxge: limit software Tx queue size
Previous implementation limits put queue size only (when Tx lock can't
be acquired), but get queue may grow unboundedly which results in mbuf
pools exhaustion and latency growth.
Submitted-by: Andrew Rybchenko <Andrew.Rybchenko at oktetlabs.ru>
Sponsored by: Solarflare Communications, Inc.
diff -r ff9f5d3dbafe -r 7632a3355224 src/driver/freebsd/sfxge_tx.c
--- a/head/sys/dev/sfxge/sfxge_tx.c Tue Mar 04 13:15:13 2014 +0400
+++ b/head/sys/dev/sfxge/sfxge_tx.c Wed Mar 05 09:06:01 2014 +0400
@@ -461,6 +461,9 @@
sfxge_tx_qdpl_swizzle(txq);
+ if (stdp->std_count >= SFXGE_TX_DPL_GET_PKT_LIMIT_DEFAULT)
+ return ENOBUFS;
+
*(stdp->std_getp) = mbuf;
stdp->std_getp = &mbuf->m_nextpkt;
stdp->std_count++;
@@ -480,7 +483,7 @@
old_len = mp->m_pkthdr.csum_data;
} else
old_len = 0;
- if (old_len >= SFXGE_TX_MAX_DEFERRED)
+ if (old_len >= SFXGE_TX_DPL_PUT_PKT_LIMIT_DEFAULT)
return ENOBUFS;
mbuf->m_pkthdr.csum_data = old_len + 1;
mbuf->m_nextpkt = (void *)old;
@@ -507,12 +510,9 @@
*/
locked = mtx_trylock(&txq->lock);
- /*
- * Can only fail if we weren't able to get the lock.
- */
if (sfxge_tx_qdpl_put(txq, m, locked) != 0) {
- KASSERT(!locked,
- ("sfxge_tx_qdpl_put() failed locked"));
+ if (locked)
+ mtx_unlock(&txq->lock);
rc = ENOBUFS;
goto fail;
}
diff -r ff9f5d3dbafe -r 7632a3355224 src/driver/freebsd/sfxge_tx.h
--- a/head/sys/dev/sfxge/sfxge_tx.h Tue Mar 04 13:15:13 2014 +0400
+++ b/head/sys/dev/sfxge/sfxge_tx.h Wed Mar 05 09:06:01 2014 +0400
@@ -75,7 +75,8 @@
enum sfxge_tx_buf_flags flags;
};
-#define SFXGE_TX_MAX_DEFERRED 64
+#define SFXGE_TX_DPL_GET_PKT_LIMIT_DEFAULT 64
+#define SFXGE_TX_DPL_PUT_PKT_LIMIT_DEFAULT 64
/*
* Deferred packet list.
--------------080407050309040104050306--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?532D62F8.4080103>