Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 12 Dec 2018 13:11:08 +0000 (UTC)
From:      Hans Petter Selasky <hselasky@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org
Subject:   svn commit: r341979 - stable/11/sys/dev/mlx5/mlx5_en
Message-ID:  <201812121311.wBCDB8El008430@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: hselasky
Date: Wed Dec 12 13:11:08 2018
New Revision: 341979
URL: https://svnweb.freebsd.org/changeset/base/341979

Log:
  MFC r341583:
  mlx5en: Statically allocate and free the channel structure(s).
  
  By allocating the worst case size channel structure array
  at attach time we can eliminate various NULL checks in the
  fast path. And also reduce the chance for use-after-free
  issues in the transmit fast path.
  
  This change is also a requirement for implementing
  backpressure support.
  
  Sponsored by:   Mellanox Technologies

Modified:
  stable/11/sys/dev/mlx5/mlx5_en/en.h
  stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_ethtool.c
  stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_main.c
  stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_tx.c
Directory Properties:
  stable/11/   (props changed)

Modified: stable/11/sys/dev/mlx5/mlx5_en/en.h
==============================================================================
--- stable/11/sys/dev/mlx5/mlx5_en/en.h	Wed Dec 12 13:08:49 2018	(r341978)
+++ stable/11/sys/dev/mlx5/mlx5_en/en.h	Wed Dec 12 13:11:08 2018	(r341979)
@@ -595,7 +595,7 @@ struct mlx5e_sq {
 #define	MLX5E_CEV_STATE_INITIAL 0	/* timer not started */
 #define	MLX5E_CEV_STATE_SEND_NOPS 1	/* send NOPs */
 #define	MLX5E_CEV_STATE_HOLD_NOPS 2	/* don't send NOPs yet */
-	u16	stopped;		/* set if SQ is stopped */
+	u16	running;		/* set if SQ is running */
 	struct callout cev_callout;
 	union {
 		u32	d32[2];
@@ -754,7 +754,6 @@ struct mlx5e_priv {
 	u32	tdn;
 	struct mlx5_core_mr mr;
 
-	struct mlx5e_channel *volatile *channel;
 	u32	tisn[MLX5E_MAX_TX_NUM_TC];
 	u32	rqtn;
 	u32	tirn[MLX5E_NUM_TT];
@@ -790,6 +789,8 @@ struct mlx5e_priv {
 	int	media_active_last;
 
 	struct callout watchdog;
+
+	struct mlx5e_channel channel[];
 };
 
 #define	MLX5E_NET_IP_ALIGN 2

Modified: stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_ethtool.c
==============================================================================
--- stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_ethtool.c	Wed Dec 12 13:08:49 2018	(r341978)
+++ stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_ethtool.c	Wed Dec 12 13:11:08 2018	(r341979)
@@ -1035,7 +1035,7 @@ mlx5e_ethtool_debug_channel_info(SYSCTL_HANDLER_ARGS)
 	if (test_bit(MLX5E_STATE_OPENED, &priv->state) == 0)
 		goto out;
 	for (i = 0; i < priv->params.num_channels; i++) {
-		c = priv->channel[i];
+		c = &priv->channel[i];
 		rq = &c->rq;
 		sbuf_printf(&sb, "channel %d rq %d cq %d\n",
 		    c->ix, rq->rqn, rq->cq.mcq.cqn);

Modified: stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_main.c
==============================================================================
--- stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_main.c	Wed Dec 12 13:08:49 2018	(r341978)
+++ stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_main.c	Wed Dec 12 13:11:08 2018	(r341979)
@@ -471,7 +471,6 @@ mlx5e_update_stats_work(struct work_struct *work)
 	    update_stats_work);
 	struct mlx5_core_dev *mdev = priv->mdev;
 	struct mlx5e_vport_stats *s = &priv->stats.vport;
-	struct mlx5e_rq_stats *rq_stats;
 	struct mlx5e_sq_stats *sq_stats;
 	struct buf_ring *sq_br;
 #if (__FreeBSD_version < 1100000)
@@ -505,10 +504,10 @@ mlx5e_update_stats_work(struct work_struct *work)
 
 	/* Collect firts the SW counters and then HW for consistency */
 	for (i = 0; i < priv->params.num_channels; i++) {
-		struct mlx5e_rq *rq = &priv->channel[i]->rq;
+		struct mlx5e_channel *pch = priv->channel + i;
+		struct mlx5e_rq *rq = &pch->rq;
+		struct mlx5e_rq_stats *rq_stats = &pch->rq.stats;
 
-		rq_stats = &priv->channel[i]->rq.stats;
-
 		/* collect stats from LRO */
 		rq_stats->sw_lro_queued = rq->lro.lro_queued;
 		rq_stats->sw_lro_flushed = rq->lro.lro_flushed;
@@ -520,8 +519,8 @@ mlx5e_update_stats_work(struct work_struct *work)
 		rx_wqe_err += rq_stats->wqe_err;
 
 		for (j = 0; j < priv->num_tc; j++) {
-			sq_stats = &priv->channel[i]->sq[j].stats;
-			sq_br = priv->channel[i]->sq[j].br;
+			sq_stats = &pch->sq[j].stats;
+			sq_br = pch->sq[j].br;
 
 			tso_packets += sq_stats->tso_packets;
 			tso_bytes += sq_stats->tso_bytes;
@@ -1097,7 +1096,7 @@ mlx5e_refresh_sq_inline(struct mlx5e_priv *priv)
 		return;
 
 	for (i = 0; i < priv->params.num_channels; i++)
-		mlx5e_refresh_sq_inline_sub(priv, priv->channel[i]);
+		mlx5e_refresh_sq_inline_sub(priv, &priv->channel[i]);
 }
 
 static int
@@ -1278,6 +1277,8 @@ mlx5e_open_sq(struct mlx5e_channel *c,
 	if (err)
 		goto err_disable_sq;
 
+	WRITE_ONCE(sq->running, 1);
+
 	return (0);
 
 err_disable_sq:
@@ -1352,20 +1353,21 @@ mlx5e_drain_sq(struct mlx5e_sq *sq)
 	/*
 	 * Check if already stopped.
 	 *
-	 * NOTE: The "stopped" variable is only written when both the
-	 * priv's configuration lock and the SQ's lock is locked. It
-	 * can therefore safely be read when only one of the two locks
-	 * is locked. This function is always called when the priv's
-	 * configuration lock is locked.
+	 * NOTE: Serialization of this function is managed by the
+	 * caller ensuring the priv's state lock is locked or in case
+	 * of rate limit support, a single thread manages drain and
+	 * resume of SQs. The "running" variable can therefore safely
+	 * be read without any locks.
 	 */
-	if (sq->stopped != 0)
+	if (READ_ONCE(sq->running) == 0)
 		return;
 
-	mtx_lock(&sq->lock);
-
 	/* don't put more packets into the SQ */
-	sq->stopped = 1;
+	WRITE_ONCE(sq->running, 0);
 
+	/* serialize access to DMA rings */
+	mtx_lock(&sq->lock);
+
 	/* teardown event factor timer, if any */
 	sq->cev_next_state = MLX5E_CEV_STATE_HOLD_NOPS;
 	callout_stop(&sq->cev_callout);
@@ -1658,15 +1660,14 @@ mlx5e_chan_mtx_destroy(struct mlx5e_channel *c)
 static int
 mlx5e_open_channel(struct mlx5e_priv *priv, int ix,
     struct mlx5e_channel_param *cparam,
-    struct mlx5e_channel *volatile *cp)
+    struct mlx5e_channel *c)
 {
-	struct mlx5e_channel *c;
 	int err;
 
-	c = malloc(sizeof(*c), M_MLX5EN, M_WAITOK | M_ZERO);
+	memset(c, 0, sizeof(*c));
+
 	c->priv = priv;
 	c->ix = ix;
-	c->cpu = 0;
 	c->ifp = priv->ifp;
 	c->mkey_be = cpu_to_be32(priv->mr.key);
 	c->num_tc = priv->num_tc;
@@ -1693,9 +1694,6 @@ mlx5e_open_channel(struct mlx5e_priv *priv, int ix,
 	if (err)
 		goto err_close_sqs;
 
-	/* store channel pointer */
-	*cp = c;
-
 	/* poll receive queue initially */
 	c->rq.cq.mcq.comp(&c->rq.cq.mcq);
 
@@ -1713,39 +1711,24 @@ err_close_tx_cqs:
 err_free:
 	/* destroy mutexes */
 	mlx5e_chan_mtx_destroy(c);
-	free(c, M_MLX5EN);
 	return (err);
 }
 
 static void
-mlx5e_close_channel(struct mlx5e_channel *volatile *pp)
+mlx5e_close_channel(struct mlx5e_channel *c)
 {
-	struct mlx5e_channel *c = *pp;
-
-	/* check if channel is already closed */
-	if (c == NULL)
-		return;
 	mlx5e_close_rq(&c->rq);
 }
 
 static void
-mlx5e_close_channel_wait(struct mlx5e_channel *volatile *pp)
+mlx5e_close_channel_wait(struct mlx5e_channel *c)
 {
-	struct mlx5e_channel *c = *pp;
-
-	/* check if channel is already closed */
-	if (c == NULL)
-		return;
-	/* ensure channel pointer is no longer used */
-	*pp = NULL;
-
 	mlx5e_close_rq_wait(&c->rq);
 	mlx5e_close_sqs_wait(c);
 	mlx5e_close_cq(&c->rq.cq);
 	mlx5e_close_tx_cqs(c);
 	/* destroy mutexes */
 	mlx5e_chan_mtx_destroy(c);
-	free(c, M_MLX5EN);
 }
 
 static int
@@ -1902,14 +1885,10 @@ static int
 mlx5e_open_channels(struct mlx5e_priv *priv)
 {
 	struct mlx5e_channel_param cparam;
-	void *ptr;
 	int err;
 	int i;
 	int j;
 
-	priv->channel = malloc(priv->params.num_channels *
-	    sizeof(struct mlx5e_channel *), M_MLX5EN, M_WAITOK | M_ZERO);
-
 	mlx5e_build_channel_param(priv, &cparam);
 	for (i = 0; i < priv->params.num_channels; i++) {
 		err = mlx5e_open_channel(priv, i, &cparam, &priv->channel[i]);
@@ -1918,7 +1897,7 @@ mlx5e_open_channels(struct mlx5e_priv *priv)
 	}
 
 	for (j = 0; j < priv->params.num_channels; j++) {
-		err = mlx5e_wait_for_min_rx_wqes(&priv->channel[j]->rq);
+		err = mlx5e_wait_for_min_rx_wqes(&priv->channel[j].rq);
 		if (err)
 			goto err_close_channels;
 	}
@@ -1926,39 +1905,22 @@ mlx5e_open_channels(struct mlx5e_priv *priv)
 	return (0);
 
 err_close_channels:
-	for (i--; i >= 0; i--) {
+	while (i--) {
 		mlx5e_close_channel(&priv->channel[i]);
 		mlx5e_close_channel_wait(&priv->channel[i]);
 	}
-
-	/* remove "volatile" attribute from "channel" pointer */
-	ptr = __DECONST(void *, priv->channel);
-	priv->channel = NULL;
-
-	free(ptr, M_MLX5EN);
-
 	return (err);
 }
 
 static void
 mlx5e_close_channels(struct mlx5e_priv *priv)
 {
-	void *ptr;
 	int i;
 
-	if (priv->channel == NULL)
-		return;
-
 	for (i = 0; i < priv->params.num_channels; i++)
 		mlx5e_close_channel(&priv->channel[i]);
 	for (i = 0; i < priv->params.num_channels; i++)
 		mlx5e_close_channel_wait(&priv->channel[i]);
-
-	/* remove "volatile" attribute from "channel" pointer */
-	ptr = __DECONST(void *, priv->channel);
-	priv->channel = NULL;
-
-	free(ptr, M_MLX5EN);
 }
 
 static int
@@ -2024,9 +1986,6 @@ mlx5e_refresh_channel_params_sub(struct mlx5e_priv *pr
 	int err;
 	int i;
 
-	if (c == NULL)
-		return (EINVAL);
-
 	err = mlx5e_refresh_rq_params(priv, &c->rq);
 	if (err)
 		goto done;
@@ -2045,13 +2004,14 @@ mlx5e_refresh_channel_params(struct mlx5e_priv *priv)
 {
 	int i;
 
-	if (priv->channel == NULL)
+	/* check if channels are closed */
+	if (test_bit(MLX5E_STATE_OPENED, &priv->state) == 0)
 		return (EINVAL);
 
 	for (i = 0; i < priv->params.num_channels; i++) {
 		int err;
 
-		err = mlx5e_refresh_channel_params_sub(priv, priv->channel[i]);
+		err = mlx5e_refresh_channel_params_sub(priv, &priv->channel[i]);
 		if (err)
 			return (err);
 	}
@@ -2145,7 +2105,7 @@ mlx5e_open_rqt(struct mlx5e_priv *priv)
 		/* apply receive side scaling stride, if any */
 		ix -= ix % (int)priv->params.channels_rsss;
 
-		MLX5_SET(rqtc, rqtc, rq_num[i], priv->channel[ix]->rq.rqn);
+		MLX5_SET(rqtc, rqtc, rq_num[i], priv->channel[ix].rq.rqn);
 	}
 
 	MLX5_SET(create_rqt_in, in, opcode, MLX5_CMD_OP_CREATE_RQT);
@@ -2212,7 +2172,7 @@ mlx5e_build_tir_ctx(struct mlx5e_priv *priv, u32 * tir
 		MLX5_SET(tirc, tirc, disp_type,
 		    MLX5_TIRC_DISP_TYPE_DIRECT);
 		MLX5_SET(tirc, tirc, inline_rqn,
-		    priv->channel[0]->rq.rqn);
+		    priv->channel[0].rq.rqn);
 		break;
 	default:
 		MLX5_SET(tirc, tirc, disp_type,
@@ -3133,7 +3093,7 @@ mlx5e_resume_sq(struct mlx5e_sq *sq)
 	int err;
 
 	/* check if already enabled */
-	if (sq->stopped == 0)
+	if (READ_ONCE(sq->running) != 0)
 		return;
 
 	err = mlx5e_modify_sq(sq, MLX5_SQC_STATE_ERR,
@@ -3156,11 +3116,8 @@ mlx5e_resume_sq(struct mlx5e_sq *sq)
 		    "mlx5e_modify_sq() from RST to RDY failed: %d\n", err);
 	}
 
-	mtx_lock(&sq->lock);
 	sq->cev_next_state = MLX5E_CEV_STATE_INITIAL;
-	sq->stopped = 0;
-	mtx_unlock(&sq->lock);
-
+	WRITE_ONCE(sq->running, 1);
 }
 
 static void
@@ -3231,18 +3188,14 @@ mlx5e_modify_tx_dma(struct mlx5e_priv *priv, uint8_t v
 {
 	int i;
 
-	if (priv->channel == NULL)
+	if (test_bit(MLX5E_STATE_OPENED, &priv->state) == 0)
 		return;
 
 	for (i = 0; i < priv->params.num_channels; i++) {
-
-		if (!priv->channel[i])
-			continue;
-
 		if (value)
-			mlx5e_disable_tx_dma(priv->channel[i]);
+			mlx5e_disable_tx_dma(&priv->channel[i]);
 		else
-			mlx5e_enable_tx_dma(priv->channel[i]);
+			mlx5e_enable_tx_dma(&priv->channel[i]);
 	}
 }
 
@@ -3251,18 +3204,14 @@ mlx5e_modify_rx_dma(struct mlx5e_priv *priv, uint8_t v
 {
 	int i;
 
-	if (priv->channel == NULL)
+	if (test_bit(MLX5E_STATE_OPENED, &priv->state) == 0)
 		return;
 
 	for (i = 0; i < priv->params.num_channels; i++) {
-
-		if (!priv->channel[i])
-			continue;
-
 		if (value)
-			mlx5e_disable_rx_dma(priv->channel[i]);
+			mlx5e_disable_rx_dma(&priv->channel[i]);
 		else
-			mlx5e_enable_rx_dma(priv->channel[i]);
+			mlx5e_enable_rx_dma(&priv->channel[i]);
 	}
 }
 
@@ -3467,7 +3416,13 @@ mlx5e_create_ifp(struct mlx5_core_dev *mdev)
 		mlx5_core_dbg(mdev, "mlx5e_check_required_hca_cap() failed\n");
 		return (NULL);
 	}
-	priv = malloc(sizeof(*priv), M_MLX5EN, M_WAITOK | M_ZERO);
+	/*
+	 * Try to allocate the priv and make room for worst-case
+	 * number of channel structures:
+	 */
+	priv = malloc(sizeof(*priv) +
+	    (sizeof(priv->channel[0]) * mdev->priv.eq_table.num_comp_vectors),
+	    M_MLX5EN, M_WAITOK | M_ZERO);
 	mlx5e_priv_mtx_init(priv);
 
 	ifp = priv->ifp = if_alloc(IFT_ETHER);

Modified: stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_tx.c
==============================================================================
--- stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_tx.c	Wed Dec 12 13:08:49 2018	(r341978)
+++ stable/11/sys/dev/mlx5/mlx5_en/mlx5_en_tx.c	Wed Dec 12 13:11:08 2018	(r341979)
@@ -81,17 +81,10 @@ static struct mlx5e_sq *
 mlx5e_select_queue(struct ifnet *ifp, struct mbuf *mb)
 {
 	struct mlx5e_priv *priv = ifp->if_softc;
-	struct mlx5e_channel * volatile *ppch;
-	struct mlx5e_channel *pch;
+	struct mlx5e_sq *sq;
 	u32 ch;
 	u32 tc;
 
-	ppch = priv->channel;
-
-	/* check if channels are successfully opened */
-	if (unlikely(ppch == NULL))
-		return (NULL);
-
 	/* obtain VLAN information if present */
 	if (mb->m_flags & M_VLANTAG) {
 		tc = (mb->m_pkthdr.ether_vtag >> 13);
@@ -127,10 +120,10 @@ mlx5e_select_queue(struct ifnet *ifp, struct mbuf *mb)
 #endif
 	}
 
-	/* check if channel is allocated and not stopped */
-	pch = ppch[ch];
-	if (likely(pch != NULL && pch->sq[tc].stopped == 0))
-		return (&pch->sq[tc]);
+	/* check if send queue is running */
+	sq = &priv->channel[ch].sq[tc];
+	if (likely(READ_ONCE(sq->running) != 0))
+		return (sq);
 	return (NULL);
 }
 
@@ -533,7 +526,7 @@ mlx5e_xmit_locked(struct ifnet *ifp, struct mlx5e_sq *
 	int err = 0;
 
 	if (unlikely((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0 ||
-	    sq->stopped != 0)) {
+	    READ_ONCE(sq->running) == 0)) {
 		m_freem(mb);
 		return (ENETDOWN);
 	}



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201812121311.wBCDB8El008430>