From owner-freebsd-net@FreeBSD.ORG Sat Mar 22 10:16:34 2014 Return-Path: Delivered-To: net@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5E67055F; Sat, 22 Mar 2014 10:16:34 +0000 (UTC) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [188.134.15.200]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id D3AACC04; Sat, 22 Mar 2014 10:16:33 +0000 (UTC) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id F14CE7F4A0; Sat, 22 Mar 2014 14:16:24 +0400 (MSK) X-DKIM: Sendmail DKIM Filter v2.8.2 shelob.oktetlabs.ru F14CE7F4A0 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1395483385; bh=74QjzOSVIaxG6qLB3U75z8Zliqd9nkzqkb3a/HYyDKI=; l=3843; h=Message-ID:Date:From:MIME-Version:To:CC:Subject:References: In-Reply-To:Content-Type; b=urBE7eoQM2w5bUPzLA3fuksByFUt/TMJuKzwxdf0kfJWijTBEnOsGvY5l4LWNMqyR NQ0wfMX1UOdo6kbQgH9XFHq3NPXZnHFJ3NZmLPdbaHolWw4uZSU9lLHEmDlGRJLtU2 OzbHj1sNAVpiJ4Db4maZs++WASUXqbonuEaP8n5E= Message-ID: <532D62F8.4080103@oktetlabs.ru> Date: Sat, 22 Mar 2014 14:16:24 +0400 From: Andrew Rybchenko Organization: OKTET Labs User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Gleb Smirnoff Subject: Re: [PATCH 2/6] sfxge: limit software Tx queue size References: <532817F5.8010505@oktetlabs.ru> <20140318132440.GG1499@FreeBSD.org> In-Reply-To: <20140318132440.GG1499@FreeBSD.org> Content-Type: multipart/mixed; boundary="------------080407050309040104050306" Cc: net@FreeBSD.org X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 22 Mar 2014 10:16:34 -0000 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 > 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 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--