Date: Wed, 16 Aug 2017 13:54:46 -0700 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Sean Bruno <sbruno@FreeBSD.org>, Matt Macy <matt@mattmacy.io> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r322338 - head/sys/net Message-ID: <20170816205446.GL1113@FreeBSD.org> In-Reply-To: <20170816204327.GK1113@FreeBSD.org> References: <201708100343.v7A3hNwR068837@repo.freebsd.org> <20170816204327.GK1113@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--0OAP2g/MAC+5xKAE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Wed, Aug 16, 2017 at 01:43:27PM -0700, Gleb Smirnoff wrote:
T> S> MPASS(m->m_nextpkt == NULL);
T> S> -
T> S> - m_free(m);
T> S> + /* if the number of clusters exceeds the number of segments
T> S> + * there won't be space on the ring to save a pointer to each
T> S> + * cluster so we simply free the list here
T> S> + */
T> S> + if (m->m_flags & M_TOOBIG) {
T> S> + m_freem(m);
T> S> + } else {
T> S> + m_free(m);
T> S> + }
T> S> ifsd_m[cidx] = NULL;
T> S> #if MEMORY_LOGGING
T> S> txq->ift_dequeued++;
T>
T> Can you please explain the goal of the change? AFAIK, the problem
T> could be fixed with one liner:
T>
T> - m_free(m);
T> + m_freem(m);
T>
T> n the iflib_tx_desc_free().
I'm probably wrong on one liner, but still I don't see reason to have flag.
Since we clear m_next in normal case, doing m_freem() always is fine.
Suggested patch attached.
--
Totus tuus, Glebius.
--0OAP2g/MAC+5xKAE
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="iflib.c.diff"
Index: sys/net/iflib.c
===================================================================
--- sys/net/iflib.c (revision 322587)
+++ sys/net/iflib.c (working copy)
@@ -267,8 +267,6 @@ iflib_get_sctx(if_ctx_t ctx)
#define RX_SW_DESC_INUSE (1 << 3)
#define TX_SW_DESC_MAPPED (1 << 4)
-#define M_TOOBIG M_UNUSED_8
-
typedef struct iflib_sw_rx_desc_array {
bus_dmamap_t *ifsd_map; /* bus_dma maps for packet */
struct mbuf **ifsd_m; /* pkthdr mbufs */
@@ -2934,7 +2932,6 @@ iflib_busdma_load_mbuf_sg(iflib_txq_t txq, bus_dma
} while (m != NULL);
if (count > *nsegs) {
ifsd_m[pidx] = *m0;
- ifsd_m[pidx]->m_flags |= M_TOOBIG;
return (0);
}
m = *m0;
@@ -3246,15 +3243,7 @@ iflib_tx_desc_free(iflib_txq_t txq, int n)
if ((m = ifsd_m[cidx]) != NULL) {
/* XXX we don't support any drivers that batch packets yet */
MPASS(m->m_nextpkt == NULL);
- /* if the number of clusters exceeds the number of segments
- * there won't be space on the ring to save a pointer to each
- * cluster so we simply free the list here
- */
- if (m->m_flags & M_TOOBIG) {
- m_freem(m);
- } else {
- m_free(m);
- }
+ m_freem(m);
ifsd_m[cidx] = NULL;
#if MEMORY_LOGGING
txq->ift_dequeued++;
--0OAP2g/MAC+5xKAE--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170816205446.GL1113>