[net-next RFC 0/1] mlx5: support per queue coalesce settings

[net-next RFC 0/1] mlx5: support per queue coalesce settings

[Nabil S. Alramli]

Hello,

I am Submitting this as an RFC to get feedback and to find out if the folks
at Mellanox would accept this change.

Currently, only gobal coalescing configuration queries or changes are
supported in the `mlx5` driver. However, per-queue operations are not, and
result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
adds support for per-queue coalesce operations, with a caveat described
below.

Here's an example use case:

- An mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned
  to a unique CPU.
- Two custom RSS contexts are created: context 1 and context 2. Each
  context has a different set of queues where flows are distributed. For
  example, context 1 may distribute flows to queues 0-3, and context 2 may
  distribute flows to queues 4-7.
- A series of ntuple filters are installed which direct matching flows to
  RSS contexts. For example, perhaps port 80 is directed to context 1 and
  port 443 to context 2.
- Applications which receive network data associated with either context
  are pinned to the CPUs where the queues in the matching context have
  their IRQs pinned to maximize locality.

The apps themselves, however, may have different requirements on latency vs
CPU usage and so setting the per queue IRQ coalesce values would be very
helpful.

This patch would support this, with the caveat that DIM mode changes per
queue are not supported. DIM mode can only be changed NIC-wide. This is
because in the mlx5 driver, global operations that change the state of
adaptive-ex or adaptive-tx require a reset. So in the case of per-queue, we
reject such requests. This was done in the interest of simplicity for this
RFC as setting the DIM mode per queue appears to require significant
changes to mlx5 to be able to preserve the state of the indvidual channels
through a reset.

IMO, if a user is going to set per-queue coalesce settings it might be
reasonable to assume that they will disable adaptive rx/tx NIC wide first
and then go through and apply their desired per-queue settings.

Here's an example:

$ ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
Queue: 2
Adaptive RX: on  TX: on
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 8
rx-frames: 128
...

Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:

$ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx off \
  rx-usecs 16
Cannot set device per queue parameters: Operation not supported

This is not supported; adaptive-rx must be disabled NIC wide first:

$ sudo ethtool -C eth0 adaptive-rx off

And now, queue_mask 0x4 can be applied to set rx-usecs:

$ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
$ ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
Queue: 2
Adaptive RX: off  TX: on
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 16
rx-frames: 32
...

Previously a global `struct mlx5e_params` stored the options in
`struct mlx5e_priv.channels.params`. That was preserved, but a channel-
specific instance was added as well, in `struct mlx5e_channel.params`.

Note that setting global coalescing options will set the individual
channel settings to the same values as well.

Is Mellanox open to this change? What would be needed to get something like
this accepted?

Best Regards,

Nabil S. Alramli (1):
  mlx5: Add {get,set}_per_queue_coalesce()

 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   3 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 212 ++++++++++++++----
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   4 +
 3 files changed, 173 insertions(+), 46 deletions(-)

-- 
2.35.1

[net-next RFC 1/1] mlx5: Add {get,set}_per_queue_coalesce()

[Nabil S. Alramli]

The mlx-5 driver currently only implements querying or modifying coalesce
configurations globally. It does not allow per-queue operations. This
patch is to implement per-queue coalesce operations in the driver.

Signed-off-by: Nabil S. Alramli <dev@nalramli.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   3 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 212 ++++++++++++++----
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   4 +
 3 files changed, 173 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index c1deb04ba7e8..e61c2fb9bc55 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -783,6 +783,7 @@ struct mlx5e_channel {
 	DECLARE_BITMAP(state, MLX5E_CHANNEL_NUM_STATES);
 	int                        ix;
 	int                        cpu;
+	struct mlx5e_params        params; /* channel specific params */
 	/* Sync between icosq recovery and XSK enable/disable. */
 	struct mutex               icosq_recovery_lock;
 };
@@ -793,7 +794,7 @@ struct mlx5e_channels {
 	struct mlx5e_channel **c;
 	struct mlx5e_ptp      *ptp;
 	unsigned int           num;
-	struct mlx5e_params    params;
+	struct mlx5e_params    params; /* global params */
 };
 
 struct mlx5e_channel_stats {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index dff02434ff45..7296ccfc0825 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -513,33 +513,55 @@ static int mlx5e_set_channels(struct net_device *dev,
 	return mlx5e_ethtool_set_channels(priv, ch);
 }
 
-int mlx5e_ethtool_get_coalesce(struct mlx5e_priv *priv,
-			       struct ethtool_coalesce *coal,
-			       struct kernel_ethtool_coalesce *kernel_coal)
+static int mlx5e_ethtool_get_per_queue_coalesce(struct mlx5e_priv *priv,
+						int queue,
+						struct ethtool_coalesce *coal,
+						struct kernel_ethtool_coalesce *kernel_coal)
 {
 	struct dim_cq_moder *rx_moder, *tx_moder;
+	struct mlx5e_params *params;
 
 	if (!MLX5_CAP_GEN(priv->mdev, cq_moderation))
 		return -EOPNOTSUPP;
 
-	rx_moder = &priv->channels.params.rx_cq_moderation;
+	if (queue != -1 && queue >= priv->channels.num) {
+		netdev_err(priv->netdev, "%s: Invalid queue ID [%d]",
+			   __func__, queue);
+		return -EINVAL;
+	}
+
+	if (queue == -1)
+		params = &priv->channels.params;
+	else
+		params = &priv->channels.c[queue]->params;
+
+	rx_moder = &params->rx_cq_moderation;
 	coal->rx_coalesce_usecs		= rx_moder->usec;
 	coal->rx_max_coalesced_frames	= rx_moder->pkts;
-	coal->use_adaptive_rx_coalesce	= priv->channels.params.rx_dim_enabled;
+	coal->use_adaptive_rx_coalesce	= params->rx_dim_enabled;
 
-	tx_moder = &priv->channels.params.tx_cq_moderation;
+	tx_moder = &params->tx_cq_moderation;
 	coal->tx_coalesce_usecs		= tx_moder->usec;
 	coal->tx_max_coalesced_frames	= tx_moder->pkts;
-	coal->use_adaptive_tx_coalesce	= priv->channels.params.tx_dim_enabled;
+	coal->use_adaptive_tx_coalesce	= params->tx_dim_enabled;
 
-	kernel_coal->use_cqe_mode_rx =
-		MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_RX_CQE_BASED_MODER);
-	kernel_coal->use_cqe_mode_tx =
-		MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_TX_CQE_BASED_MODER);
+	if (kernel_coal) {
+		kernel_coal->use_cqe_mode_rx =
+			MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_RX_CQE_BASED_MODER);
+		kernel_coal->use_cqe_mode_tx =
+			MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_TX_CQE_BASED_MODER);
+	}
 
 	return 0;
 }
 
+int mlx5e_ethtool_get_coalesce(struct mlx5e_priv *priv,
+			       struct ethtool_coalesce *coal,
+			       struct kernel_ethtool_coalesce *kernel_coal)
+{
+	return mlx5e_ethtool_get_per_queue_coalesce(priv, -1, coal, kernel_coal);
+}
+
 static int mlx5e_get_coalesce(struct net_device *netdev,
 			      struct ethtool_coalesce *coal,
 			      struct kernel_ethtool_coalesce *kernel_coal,
@@ -550,36 +572,76 @@ static int mlx5e_get_coalesce(struct net_device *netdev,
 	return mlx5e_ethtool_get_coalesce(priv, coal, kernel_coal);
 }
 
+/**
+ * mlx5e_get_per_queue_coalesce - gets coalesce settings for particular queue
+ * @netdev: netdev structure
+ * @coal: ethtool's coalesce settings
+ * @queue: the particular queue to read
+ *
+ * Reads a specific queue's coalesce settings
+ **/
+static int mlx5e_get_per_queue_coalesce(struct net_device *netdev,
+					u32 queue,
+					struct ethtool_coalesce *coal)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	return mlx5e_ethtool_get_per_queue_coalesce(priv, queue, coal, NULL);
+}
+
 #define MLX5E_MAX_COAL_TIME		MLX5_MAX_CQ_PERIOD
 #define MLX5E_MAX_COAL_FRAMES		MLX5_MAX_CQ_COUNT
 
 static void
-mlx5e_set_priv_channels_tx_coalesce(struct mlx5e_priv *priv, struct ethtool_coalesce *coal)
+mlx5e_set_priv_channels_tx_coalesce(struct mlx5e_priv *priv,
+				    int queue,
+				    struct ethtool_coalesce *coal)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	int tc;
 	int i;
 
-	for (i = 0; i < priv->channels.num; ++i) {
-		struct mlx5e_channel *c = priv->channels.c[i];
+	if (queue == -1) {
+		for (i = 0; i < priv->channels.num; ++i) {
+			struct mlx5e_channel *c = priv->channels.c[i];
+
+			for (tc = 0; tc < c->num_tc; tc++) {
+				mlx5_core_modify_cq_moderation(mdev,
+							       &c->sq[tc].cq.mcq,
+							       coal->tx_coalesce_usecs,
+							       coal->tx_max_coalesced_frames);
+			}
+		}
+	} else {
+		struct mlx5e_channel *c = priv->channels.c[queue];
 
 		for (tc = 0; tc < c->num_tc; tc++) {
 			mlx5_core_modify_cq_moderation(mdev,
-						&c->sq[tc].cq.mcq,
-						coal->tx_coalesce_usecs,
-						coal->tx_max_coalesced_frames);
+						       &c->sq[tc].cq.mcq,
+						       coal->tx_coalesce_usecs,
+						       coal->tx_max_coalesced_frames);
 		}
 	}
 }
 
 static void
-mlx5e_set_priv_channels_rx_coalesce(struct mlx5e_priv *priv, struct ethtool_coalesce *coal)
+mlx5e_set_priv_channels_rx_coalesce(struct mlx5e_priv *priv,
+				    int queue,
+				    struct ethtool_coalesce *coal)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	int i;
 
-	for (i = 0; i < priv->channels.num; ++i) {
-		struct mlx5e_channel *c = priv->channels.c[i];
+	if (queue == -1) {
+		for (i = 0; i < priv->channels.num; ++i) {
+			struct mlx5e_channel *c = priv->channels.c[i];
+
+			mlx5_core_modify_cq_moderation(mdev, &c->rq.cq.mcq,
+						       coal->rx_coalesce_usecs,
+						       coal->rx_max_coalesced_frames);
+		}
+	} else {
+		struct mlx5e_channel *c = priv->channels.c[queue];
 
 		mlx5_core_modify_cq_moderation(mdev, &c->rq.cq.mcq,
 					       coal->rx_coalesce_usecs,
@@ -596,15 +658,17 @@ static int cqe_mode_to_period_mode(bool val)
 	return val ? MLX5_CQ_PERIOD_MODE_START_FROM_CQE : MLX5_CQ_PERIOD_MODE_START_FROM_EQE;
 }
 
-int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
-			       struct ethtool_coalesce *coal,
-			       struct kernel_ethtool_coalesce *kernel_coal,
-			       struct netlink_ext_ack *extack)
+static int mlx5e_ethtool_set_per_queue_coalesce(struct mlx5e_priv *priv,
+						int queue,
+						struct ethtool_coalesce *coal,
+						struct kernel_ethtool_coalesce *kernel_coal,
+						struct netlink_ext_ack *extack)
 {
 	struct dim_cq_moder *rx_moder, *tx_moder;
 	struct mlx5_core_dev *mdev = priv->mdev;
-	struct mlx5e_params new_params;
-	bool reset_rx, reset_tx;
+	bool reset_rx = false, reset_tx = false;
+	struct mlx5e_params new_params = {0};
+	struct mlx5e_params *old_params;
 	bool reset = true;
 	u8 cq_period_mode;
 	int err = 0;
@@ -626,14 +690,29 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 		return -ERANGE;
 	}
 
-	if ((kernel_coal->use_cqe_mode_rx || kernel_coal->use_cqe_mode_tx) &&
-	    !MLX5_CAP_GEN(priv->mdev, cq_period_start_from_cqe)) {
-		NL_SET_ERR_MSG_MOD(extack, "cqe_mode_rx/tx is not supported on this device");
-		return -EOPNOTSUPP;
+	if (kernel_coal) {
+		if ((kernel_coal->use_cqe_mode_rx || kernel_coal->use_cqe_mode_tx) &&
+		    !MLX5_CAP_GEN(priv->mdev, cq_period_start_from_cqe)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "cqe_mode_rx/tx is not supported on this device");
+			return -EOPNOTSUPP;
+		}
+	}
+
+	if (queue != -1 && queue >= priv->channels.num) {
+		netdev_err(priv->netdev, "%s: Invalid queue ID [%d]",
+			   __func__, queue);
+		return -EINVAL;
 	}
 
 	mutex_lock(&priv->state_lock);
-	new_params = priv->channels.params;
+
+	if (queue == -1)
+		old_params = &priv->channels.params;
+	else
+		old_params = &priv->channels.c[queue]->params;
+
+	new_params = *old_params;
 
 	rx_moder          = &new_params.rx_cq_moderation;
 	rx_moder->usec    = coal->rx_coalesce_usecs;
@@ -645,19 +724,21 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 	tx_moder->pkts    = coal->tx_max_coalesced_frames;
 	new_params.tx_dim_enabled = !!coal->use_adaptive_tx_coalesce;
 
-	reset_rx = !!coal->use_adaptive_rx_coalesce != priv->channels.params.rx_dim_enabled;
-	reset_tx = !!coal->use_adaptive_tx_coalesce != priv->channels.params.tx_dim_enabled;
+	reset_rx = !!coal->use_adaptive_rx_coalesce != old_params->rx_dim_enabled;
+	reset_tx = !!coal->use_adaptive_tx_coalesce != old_params->tx_dim_enabled;
 
-	cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_rx);
-	if (cq_period_mode != rx_moder->cq_period_mode) {
-		mlx5e_set_rx_cq_mode_params(&new_params, cq_period_mode);
-		reset_rx = true;
-	}
+	if (kernel_coal) {
+		cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_rx);
+		if (cq_period_mode != rx_moder->cq_period_mode) {
+			mlx5e_set_rx_cq_mode_params(&new_params, cq_period_mode);
+			reset_rx = true;
+		}
 
-	cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_tx);
-	if (cq_period_mode != tx_moder->cq_period_mode) {
-		mlx5e_set_tx_cq_mode_params(&new_params, cq_period_mode);
-		reset_tx = true;
+		cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_tx);
+		if (cq_period_mode != tx_moder->cq_period_mode) {
+			mlx5e_set_tx_cq_mode_params(&new_params, cq_period_mode);
+			reset_tx = true;
+		}
 	}
 
 	if (reset_rx) {
@@ -678,18 +759,40 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 	 */
 	if (!reset_rx && !reset_tx && test_bit(MLX5E_STATE_OPENED, &priv->state)) {
 		if (!coal->use_adaptive_rx_coalesce)
-			mlx5e_set_priv_channels_rx_coalesce(priv, coal);
+			mlx5e_set_priv_channels_rx_coalesce(priv, queue, coal);
 		if (!coal->use_adaptive_tx_coalesce)
-			mlx5e_set_priv_channels_tx_coalesce(priv, coal);
+			mlx5e_set_priv_channels_tx_coalesce(priv, queue, coal);
 		reset = false;
 	}
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset);
+	if (queue == -1) {
+		err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset);
+	} else {
+		if (reset) {
+			netdev_err(priv->netdev, "%s: Per-queue adaptive-rx / adaptive-tx operations are not supported",
+				   __func__);
+			err = -EOPNOTSUPP;
+		} else {
+			/* Since preactivate is NULL and we are not doing reset we just copy the
+			 * params here instead of calling mlx5e_safe_switch_params() to avoid
+			 * having to pass the queue to it.
+			 */
+			priv->channels.c[queue]->params = new_params;
+		}
+	}
 
 	mutex_unlock(&priv->state_lock);
 	return err;
 }
 
+int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
+			       struct ethtool_coalesce *coal,
+			       struct kernel_ethtool_coalesce *kernel_coal,
+			       struct netlink_ext_ack *extack)
+{
+	return mlx5e_ethtool_set_per_queue_coalesce(priv, -1, coal, kernel_coal, extack);
+}
+
 static int mlx5e_set_coalesce(struct net_device *netdev,
 			      struct ethtool_coalesce *coal,
 			      struct kernel_ethtool_coalesce *kernel_coal,
@@ -700,6 +803,23 @@ static int mlx5e_set_coalesce(struct net_device *netdev,
 	return mlx5e_ethtool_set_coalesce(priv, coal, kernel_coal, extack);
 }
 
+/**
+ * mlx5e_set_per_queue_coalesce - set specific queue's coalesce settings
+ * @netdev: the netdev to change
+ * @coal: ethtool's coalesce settings
+ * @queue: the queue to change
+ *
+ * Sets the specified queue's coalesce settings.
+ **/
+static int mlx5e_set_per_queue_coalesce(struct net_device *netdev,
+					u32 queue,
+					struct ethtool_coalesce *coal)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	return mlx5e_ethtool_set_per_queue_coalesce(priv, queue, coal, NULL, NULL);
+}
+
 static void ptys2ethtool_supported_link(struct mlx5_core_dev *mdev,
 					unsigned long *supported_modes,
 					u32 eth_proto_cap)
@@ -2434,6 +2554,8 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.flash_device      = mlx5e_flash_device,
 	.get_priv_flags    = mlx5e_get_priv_flags,
 	.set_priv_flags    = mlx5e_set_priv_flags,
+	.get_per_queue_coalesce	= mlx5e_get_per_queue_coalesce,
+	.set_per_queue_coalesce	= mlx5e_set_per_queue_coalesce,
 	.self_test         = mlx5e_self_test,
 	.get_fec_stats     = mlx5e_get_fec_stats,
 	.get_fecparam      = mlx5e_get_fecparam,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index a2ae791538ed..4fce06ca57bc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3043,6 +3043,7 @@ int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
 {
 	struct mlx5e_channels *new_chs;
 	int err;
+	int i;
 
 	reset &= test_bit(MLX5E_STATE_OPENED, &priv->state);
 	if (!reset)
@@ -3059,6 +3060,9 @@ int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
 	if (err)
 		goto err_cancel_selq;
 
+	for (i = 0; i < new_chs->num; ++i)
+		new_chs->c[i]->params = *params;
+
 	err = mlx5e_switch_priv_channels(priv, new_chs, preactivate, context);
 	if (err)
 		goto err_close;
-- 
2.35.1

Re: [net-next RFC 0/1] mlx5: support per queue coalesce settings

[Saeed Mahameed]

On 23 Aug 18:31, Nabil S. Alramli wrote:
>Hello,
>
>I am Submitting this as an RFC to get feedback and to find out if the folks
>at Mellanox would accept this change.
>
>Currently, only gobal coalescing configuration queries or changes are
>supported in the `mlx5` driver. However, per-queue operations are not, and
>result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
>adds support for per-queue coalesce operations, with a caveat described
>below.
>
>Here's an example use case:
>
>- An mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned
>  to a unique CPU.
>- Two custom RSS contexts are created: context 1 and context 2. Each
>  context has a different set of queues where flows are distributed. For
>  example, context 1 may distribute flows to queues 0-3, and context 2 may
>  distribute flows to queues 4-7.
>- A series of ntuple filters are installed which direct matching flows to
>  RSS contexts. For example, perhaps port 80 is directed to context 1 and
>  port 443 to context 2.
>- Applications which receive network data associated with either context
>  are pinned to the CPUs where the queues in the matching context have
>  their IRQs pinned to maximize locality.
>
>The apps themselves, however, may have different requirements on latency vs
>CPU usage and so setting the per queue IRQ coalesce values would be very
>helpful.
>
>This patch would support this, with the caveat that DIM mode changes per
>queue are not supported. DIM mode can only be changed NIC-wide. This is
>because in the mlx5 driver, global operations that change the state of
>adaptive-ex or adaptive-tx require a reset. So in the case of per-queue, we
>reject such requests. This was done in the interest of simplicity for this
>RFC as setting the DIM mode per queue appears to require significant
>changes to mlx5 to be able to preserve the state of the indvidual channels
>through a reset.
>
>IMO, if a user is going to set per-queue coalesce settings it might be
>reasonable to assume that they will disable adaptive rx/tx NIC wide first
>and then go through and apply their desired per-queue settings.
>

DIM is already per channel, so I don't think it's that difficult to have
mix support of DIM and static config per channel. Yes the code will need
some refactoring, but with a quick glance at the code provided here, such
refactoring is already required IMO.

>Here's an example:
>
>$ ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>Queue: 2
>Adaptive RX: on  TX: on
>stats-block-usecs: 0
>sample-interval: 0
>pkt-rate-low: 0
>pkt-rate-high: 0
>
>rx-usecs: 8
>rx-frames: 128
>...
>
>Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:
>
>$ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx off \
>  rx-usecs 16
>Cannot set device per queue parameters: Operation not supported
>
>This is not supported; adaptive-rx must be disabled NIC wide first:
>
>$ sudo ethtool -C eth0 adaptive-rx off
>
>And now, queue_mask 0x4 can be applied to set rx-usecs:
>
>$ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
>$ ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>Queue: 2
>Adaptive RX: off  TX: on
>stats-block-usecs: 0
>sample-interval: 0
>pkt-rate-low: 0
>pkt-rate-high: 0
>
>rx-usecs: 16
>rx-frames: 32
>...
>
>Previously a global `struct mlx5e_params` stored the options in
>`struct mlx5e_priv.channels.params`. That was preserved, but a channel-
>specific instance was added as well, in `struct mlx5e_channel.params`.
>
>Note that setting global coalescing options will set the individual
>channel settings to the same values as well.
>
>Is Mellanox open to this change? What would be needed to get something like
>this accepted?
>

Sure, we just need to pass review and few testing cycles.

Thanks,
Saeed.

Re: [net-next RFC 0/1] mlx5: support per queue coalesce settings

[Nabil S. Alramli]

Hi Saeed,

Thank you for your review and feedback. Please see below for a couple of 
additional questions.

On 8/24/23 14:51, Saeed Mahameed wrote:
> On 23 Aug 18:31, Nabil S. Alramli wrote:
>> Hello,
>>
>> I am Submitting this as an RFC to get feedback and to find out if the 
>> folks
>> at Mellanox would accept this change.
>>
>> Currently, only gobal coalescing configuration queries or changes are
>> supported in the `mlx5` driver. However, per-queue operations are not, 
>> and
>> result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
>> adds support for per-queue coalesce operations, with a caveat described
>> below.
>>
>> Here's an example use case:
>>
>> - An mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned
>>  to a unique CPU.
>> - Two custom RSS contexts are created: context 1 and context 2. Each
>>  context has a different set of queues where flows are distributed. For
>>  example, context 1 may distribute flows to queues 0-3, and context 2 may
>>  distribute flows to queues 4-7.
>> - A series of ntuple filters are installed which direct matching flows to
>>  RSS contexts. For example, perhaps port 80 is directed to context 1 and
>>  port 443 to context 2.
>> - Applications which receive network data associated with either context
>>  are pinned to the CPUs where the queues in the matching context have
>>  their IRQs pinned to maximize locality.
>>
>> The apps themselves, however, may have different requirements on 
>> latency vs
>> CPU usage and so setting the per queue IRQ coalesce values would be very
>> helpful.
>>
>> This patch would support this, with the caveat that DIM mode changes per
>> queue are not supported. DIM mode can only be changed NIC-wide. This is
>> because in the mlx5 driver, global operations that change the state of
>> adaptive-ex or adaptive-tx require a reset. So in the case of 
>> per-queue, we
>> reject such requests. This was done in the interest of simplicity for 
>> this
>> RFC as setting the DIM mode per queue appears to require significant
>> changes to mlx5 to be able to preserve the state of the indvidual 
>> channels
>> through a reset.
>>
>> IMO, if a user is going to set per-queue coalesce settings it might be
>> reasonable to assume that they will disable adaptive rx/tx NIC wide first
>> and then go through and apply their desired per-queue settings.
>>
> 
> DIM is already per channel, so I don't think it's that difficult to have
> mix support of DIM and static config per channel. Yes the code will need
> some refactoring, but with a quick glance at the code provided here, such
> refactoring is already required IMO.
> 
That sounds good. I'll go ahead and add support for DIM per-channel 
coalescing updates. I'll send another RFC when that is ready for initial 
feedback.

Can you please provide some clarification as to what you mean that some 
refactoring is needed? Are you referring to the existing code, or is 
there something specifically in this patch?

For example, I think we'll need to make some additional changes to 
mlx5e_safe_switch_params(), mlx5e_open_channels() and probably more. Is 
this what you have in mind?

>> Here's an example:
>>
>> $ ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: on  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> ...
>>
>> Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:
>>
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx 
>> off \
>>  rx-usecs 16
>> Cannot set device per queue parameters: Operation not supported
>>
>> This is not supported; adaptive-rx must be disabled NIC wide first:
>>
>> $ sudo ethtool -C eth0 adaptive-rx off
>>
>> And now, queue_mask 0x4 can be applied to set rx-usecs:
>>
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
>> $ ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: off  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 16
>> rx-frames: 32
>> ...
>>
>> Previously a global `struct mlx5e_params` stored the options in
>> `struct mlx5e_priv.channels.params`. That was preserved, but a channel-
>> specific instance was added as well, in `struct mlx5e_channel.params`.
>>
>> Note that setting global coalescing options will set the individual
>> channel settings to the same values as well.
>>
>> Is Mellanox open to this change? What would be needed to get something 
>> like
>> this accepted?
>>
> 
> Sure, we just need to pass review and few testing cycles.
> 
> Thanks,
> Saeed.
> 

Best Regards,

-- Nabil S. Alramli

[net-next RFC v2 0/4] mlx5: support per queue coalesce settings

[Nabil S. Alramli]

Hello,

This is v2 of my previous patch:
https://lore.kernel.org/lkml/20230823223121.58676-1-dev@nalramli.com/.

Saeed: Thanks for reviewing v1. I made significant changes to support
per-channel DIM settings. Is this ready for an official v1 submission or
are there other major changes you'd like to see before I do that?

***************************************************************************
Version History
---------------
* v1: Initial draft, individual channel DIM changes not supported.
* v2: Support individual channel DIM changes.
***************************************************************************

Currently, only gobal coalescing configuration queries or changes are
supported in the `mlx5` driver. However, per-queue operations are not, and
result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
adds support for per-queue coalesce operations.

Here's an example use case:

- A mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned to
  a unique CPU.
- Two custom RSS contexts are created: context 1 and context 2. Each
  context has a different set of queues where flows are distributed. For
  example, context 1 may distribute flows to queues 0-3, and context 2 may
  distribute flows to queues 4-7.
- A series of ntuple filters are installed which direct matching flows to
  RSS contexts. For example, perhaps port 80 is directed to context 1 and
  port 443 to context 2.
- Applications which receive network data associated with either context
  are pinned to the CPUs where the queues in the matching context have
  their IRQs pinned to maximize locality.

The apps themselves, however, may have different requirements on latency vs
CPU usage and so setting the per queue IRQ coalesce values would be very
helpful.

This patch would support this. In v1 DIM mode changes could only be changed
NIC-wide. However, in this iteration, DIM mode changes are supported
globally as well as on a per-queue basis.

Here's an example:

```
$ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
Queue: 2
Adaptive RX: on  TX: on
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 8
rx-frames: 128
rx-usecs-irq: 0
rx-frames-irq: 0

tx-usecs: 8
tx-frames: 128
tx-usecs-irq: 0
tx-frames-irq: 0
```

Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:

```
$ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx off
$ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
```

Confirm that the operation succeeded:

```
$ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
Queue: 2
Adaptive RX: off  TX: on
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 16
rx-frames: 32
rx-usecs-irq: 0
rx-frames-irq: 0

tx-usecs: 8
tx-frames: 128
tx-usecs-irq: 0
tx-frames-irq: 0
```

The individual channel settings do not overwrite the global ones. However
Setting the global parameters will also reset all of the individual channel
options. For example, after we set the options for queue 2, we'll see that
the global options remain unchanged:
```
$ sudo ethtool --show-coalesce eth0
Coalesce parameters for eth0:
Adaptive RX: on  TX: on
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 8
rx-frames: 128
rx-usecs-irq: 0
rx-frames-irq: 0

tx-usecs: 16
tx-frames: 32
tx-usecs-irq: 0
tx-frames-irq: 0
```

But then if we set them, we'll see that the options for queue 2 have been
reset as well:
```
$ sudo ethtool --coalesce eth0 adaptive-tx off

$ sudo ethtool --show-coalesce eth0
Coalesce parameters for eth0:
Adaptive RX: on  TX: off
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 8
rx-frames: 128
rx-usecs-irq: 0
rx-frames-irq: 0

tx-usecs: 16
tx-frames: 32
tx-usecs-irq: 0
tx-frames-irq: 0

$ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
Queue: 2
Adaptive RX: on  TX: off
stats-block-usecs: 0
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0

rx-usecs: 8
rx-frames: 128
rx-usecs-irq: 0
rx-frames-irq: 0

tx-usecs: 16
tx-frames: 32
tx-usecs-irq: 0
tx-frames-irq: 0
```

Previously a global `struct mlx5e_params` stored the options in
`struct mlx5e_priv.channels.params`. That was preserved, but a channel-
specific instance was added as well, in `struct mlx5e_channel.params`.

Best Regards,

***************************************************************************

Nabil S. Alramli (4):
  mlx5: Add mlx5e_param to individual mlx5e_channel and preserve them
    through mlx5e_open_channels()
  mlx5: Add queue number parameter to mlx5e_safe_switch_params()
  mlx5: Implement mlx5e_ethtool_{get,set}_per_queue_coalesce() to
    support per-queue operations
  mlx5: Add {get,set}_per_queue_coalesce()

 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   6 +-
 .../ethernet/mellanox/mlx5/core/en_dcbnl.c    |   2 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 214 +++++++++++++-----
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  76 +++++--
 .../ethernet/mellanox/mlx5/core/ipoib/ipoib.c |   2 +-
 5 files changed, 222 insertions(+), 78 deletions(-)

-- 
2.35.1

[net-next RFC v2 1/4] mlx5: Add mlx5e_param to individual mlx5e_channel and preserve them through mlx5e_open_channels()

[Nabil S. Alramli]

The mlx-5 driver currently has one global `struct mlx5e_param` for storing
settings including coalescing ones which means that individual channels
cannot have their own independent settings. This patch adds a
`struct mlx5e_param` to each `struct mlx5e_channel`. The global settings
have been preserved as well to continue to support global commands.

Signed-off-by: Nabil S. Alramli <dev@nalramli.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  3 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 34 ++++++++++++++-----
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 86f2690c5e01..3657839b88f8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -783,6 +783,7 @@ struct mlx5e_channel {
 	DECLARE_BITMAP(state, MLX5E_CHANNEL_NUM_STATES);
 	int                        ix;
 	int                        cpu;
+	struct mlx5e_params        params; /* channel specific params */
 	/* Sync between icosq recovery and XSK enable/disable. */
 	struct mutex               icosq_recovery_lock;
 };
@@ -793,7 +794,7 @@ struct mlx5e_channels {
 	struct mlx5e_channel **c;
 	struct mlx5e_ptp      *ptp;
 	unsigned int           num;
-	struct mlx5e_params    params;
+	struct mlx5e_params    params; /* global params */
 };
 
 struct mlx5e_channel_stats {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index a2ae791538ed..a1578219187b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2471,6 +2471,7 @@ static int mlx5e_open_channel(struct mlx5e_priv *priv, int ix,
 	c->tstamp   = &priv->tstamp;
 	c->ix       = ix;
 	c->cpu      = cpu;
+	c->params   = *params;
 	c->pdev     = mlx5_core_dma_dev(priv->mdev);
 	c->netdev   = priv->netdev;
 	c->mkey_be  = cpu_to_be32(priv->mdev->mlx5e_res.hw_objs.mkey);
@@ -2558,27 +2559,42 @@ int mlx5e_open_channels(struct mlx5e_priv *priv,
 			struct mlx5e_channels *chs)
 {
 	struct mlx5e_channel_param *cparam;
+	bool use_global_params = true;
 	int err = -ENOMEM;
 	int i;
 
 	chs->num = chs->params.num_channels;
 
+	/* This check is necessary here in case chs == priv->channels because the allocation of
+	 * chs->c will invalidate it
+	 */
+	if (priv->channels.c)
+		use_global_params = false;
+
 	chs->c = kcalloc(chs->num, sizeof(struct mlx5e_channel *), GFP_KERNEL);
-	cparam = kvzalloc(sizeof(struct mlx5e_channel_param), GFP_KERNEL);
+	cparam = kvmalloc(sizeof(*cparam), GFP_KERNEL);
 	if (!chs->c || !cparam)
 		goto err_free;
 
-	err = mlx5e_build_channel_param(priv->mdev, &chs->params, priv->q_counter, cparam);
-	if (err)
-		goto err_free;
-
 	for (i = 0; i < chs->num; i++) {
 		struct xsk_buff_pool *xsk_pool = NULL;
+		struct mlx5e_params *params = NULL;
+
+		/* If priv->channels.c is not NULL, then retain the original params */
+		if (use_global_params)
+			params = &chs->params;
+		else
+			params = &priv->channels.c[i]->params;
+
+		memset(cparam, 0, sizeof(*cparam));
+		err = mlx5e_build_channel_param(priv->mdev, params, priv->q_counter, cparam);
+		if (err)
+			goto err_free;
 
-		if (chs->params.xdp_prog)
-			xsk_pool = mlx5e_xsk_get_pool(&chs->params, chs->params.xsk, i);
+		if (params->xdp_prog)
+			xsk_pool = mlx5e_xsk_get_pool(params, params->xsk, i);
 
-		err = mlx5e_open_channel(priv, i, &chs->params, cparam, xsk_pool, &chs->c[i]);
+		err = mlx5e_open_channel(priv, i, params, cparam, xsk_pool, &chs->c[i]);
 		if (err)
 			goto err_close_channels;
 	}
@@ -3115,6 +3131,7 @@ int mlx5e_open_locked(struct net_device *netdev)
 
 	set_bit(MLX5E_STATE_OPENED, &priv->state);
 
+	priv->channels.c  = NULL;
 	err = mlx5e_open_channels(priv, &priv->channels);
 	if (err)
 		goto err_clear_state_opened_flag;
@@ -5611,6 +5628,7 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
 	priv->netdev      = netdev;
 	priv->max_nch     = nch;
 	priv->max_opened_tc = 1;
+	priv->channels.c  = NULL;
 
 	if (!alloc_cpumask_var(&priv->scratchpad.cpumask, GFP_KERNEL))
 		return -ENOMEM;
-- 
2.35.1

[net-next RFC v2 2/4] mlx5: Add queue number parameter to mlx5e_safe_switch_params()

[Nabil S. Alramli]

Currently mlx5e_safe_switch_params() does not take a queue number and
assumes a global operation. This allows the function to handle individual
channels. Note that Global operations reset the individual channel
settings.

Signed-off-by: Nabil S. Alramli <dev@nalramli.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  3 +-
 .../ethernet/mellanox/mlx5/core/en_dcbnl.c    |  2 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 18 ++++----
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 42 +++++++++++++------
 .../ethernet/mellanox/mlx5/core/ipoib/ipoib.c |  2 +-
 5 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 3657839b88f8..ba8eb2b30251 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -1063,7 +1063,8 @@ int mlx5e_safe_reopen_channels(struct mlx5e_priv *priv);
 int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
 			     struct mlx5e_params *new_params,
 			     mlx5e_fp_preactivate preactivate,
-			     void *context, bool reset);
+			     void *context, bool reset,
+			     int queue);
 int mlx5e_update_tx_netdev_queues(struct mlx5e_priv *priv);
 int mlx5e_num_channels_changed_ctx(struct mlx5e_priv *priv, void *context);
 void mlx5e_activate_priv_channels(struct mlx5e_priv *priv);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
index 8705cffc747f..8c0704beb25c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
@@ -1159,7 +1159,7 @@ static int mlx5e_set_trust_state(struct mlx5e_priv *priv, u8 trust_state)
 
 	err = mlx5e_safe_switch_params(priv, &new_params,
 				       mlx5e_update_trust_state_hw,
-				       &trust_state, reset);
+				       &trust_state, reset, -1);
 
 	mutex_unlock(&priv->state_lock);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index dff02434ff45..13e5838ff1ee 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -389,7 +389,7 @@ int mlx5e_ethtool_set_ringparam(struct mlx5e_priv *priv,
 	if (err)
 		goto unlock;
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true, -1);
 
 unlock:
 	mutex_unlock(&priv->state_lock);
@@ -489,7 +489,7 @@ int mlx5e_ethtool_set_channels(struct mlx5e_priv *priv,
 
 	/* Switch to new channels, set new parameters and close old ones */
 	err = mlx5e_safe_switch_params(priv, &new_params,
-				       mlx5e_num_channels_changed_ctx, NULL, true);
+				       mlx5e_num_channels_changed_ctx, NULL, true, -1);
 
 	if (arfs_enabled) {
 		int err2 = mlx5e_arfs_enable(priv->fs);
@@ -684,7 +684,7 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 		reset = false;
 	}
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset, -1);
 
 	mutex_unlock(&priv->state_lock);
 	return err;
@@ -1893,7 +1893,7 @@ static int set_pflag_cqe_based_moder(struct net_device *netdev, bool enable,
 	else
 		mlx5e_set_tx_cq_mode_params(&new_params, cq_period_mode);
 
-	return mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	return mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true, -1);
 }
 
 static int set_pflag_tx_cqe_based_moder(struct net_device *netdev, bool enable)
@@ -1935,10 +1935,10 @@ int mlx5e_modify_rx_cqe_compression_locked(struct mlx5e_priv *priv, bool new_val
 		new_params.ptp_rx = new_val;
 
 	if (new_params.ptp_rx == priv->channels.params.ptp_rx)
-		err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+		err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true, -1);
 	else
 		err = mlx5e_safe_switch_params(priv, &new_params, mlx5e_ptp_rx_manage_fs_ctx,
-					       &new_params.ptp_rx, true);
+					       &new_params.ptp_rx, true, -1);
 	if (err)
 		return err;
 
@@ -1996,7 +1996,7 @@ static int set_pflag_rx_striding_rq(struct net_device *netdev, bool enable)
 	MLX5E_SET_PFLAG(&new_params, MLX5E_PFLAG_RX_STRIDING_RQ, enable);
 	mlx5e_set_rq_type(mdev, &new_params);
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true, -1);
 	if (err)
 		return err;
 
@@ -2041,7 +2041,7 @@ static int set_pflag_tx_mpwqe_common(struct net_device *netdev, u32 flag, bool e
 
 	MLX5E_SET_PFLAG(&new_params, flag, enable);
 
-	return mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	return mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true, -1);
 }
 
 static int set_pflag_xdp_tx_mpwqe(struct net_device *netdev, bool enable)
@@ -2094,7 +2094,7 @@ static int set_pflag_tx_port_ts(struct net_device *netdev, bool enable)
 	 */
 
 	err = mlx5e_safe_switch_params(priv, &new_params,
-				       mlx5e_num_channels_changed_ctx, NULL, true);
+				       mlx5e_num_channels_changed_ctx, NULL, true, -1);
 	if (!err)
 		priv->tx_ptp_opened = true;
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index a1578219187b..d7325fc4f2c0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3055,19 +3055,35 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
 int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
 			     struct mlx5e_params *params,
 			     mlx5e_fp_preactivate preactivate,
-			     void *context, bool reset)
+			     void *context, bool reset,
+			     int queue)
 {
 	struct mlx5e_channels *new_chs;
 	int err;
+	int i;
+
+	if (queue == -1) {
+		for (i = 0; i < priv->channels.num; i++)
+			priv->channels.c[i]->params = *params;
+	} else {
+		priv->channels.c[queue]->params = *params;
+	}
 
 	reset &= test_bit(MLX5E_STATE_OPENED, &priv->state);
-	if (!reset)
-		return mlx5e_switch_priv_params(priv, params, preactivate, context);
+	if (!reset) {
+		if (queue == -1)
+			return mlx5e_switch_priv_params(priv, params, preactivate, context);
+		else
+			return 0;
+	}
 
 	new_chs = kzalloc(sizeof(*new_chs), GFP_KERNEL);
 	if (!new_chs)
 		return -ENOMEM;
-	new_chs->params = *params;
+	if (queue == -1)
+		new_chs->params = *params;
+	else
+		new_chs->params = priv->channels.params;
 
 	mlx5e_selq_prepare_params(&priv->selq, &new_chs->params);
 
@@ -3093,7 +3109,7 @@ int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
 
 int mlx5e_safe_reopen_channels(struct mlx5e_priv *priv)
 {
-	return mlx5e_safe_switch_params(priv, &priv->channels.params, NULL, NULL, true);
+	return mlx5e_safe_switch_params(priv, &priv->channels.params, NULL, NULL, true, -1);
 }
 
 void mlx5e_timestamp_init(struct mlx5e_priv *priv)
@@ -3486,7 +3502,7 @@ static int mlx5e_setup_tc_mqprio_dcb(struct mlx5e_priv *priv,
 	mlx5e_params_mqprio_dcb_set(&new_params, tc ? tc : 1);
 
 	err = mlx5e_safe_switch_params(priv, &new_params,
-				       mlx5e_num_channels_changed_ctx, NULL, true);
+				       mlx5e_num_channels_changed_ctx, NULL, true, -1);
 
 	if (!err && priv->mqprio_rl) {
 		mlx5e_mqprio_rl_cleanup(priv->mqprio_rl);
@@ -3607,7 +3623,7 @@ static int mlx5e_setup_tc_mqprio_channel(struct mlx5e_priv *priv,
 	nch_changed = mlx5e_get_dcb_num_tc(&priv->channels.params) > 1;
 	preactivate = nch_changed ? mlx5e_num_channels_changed_ctx :
 		mlx5e_update_netdev_queues_ctx;
-	err = mlx5e_safe_switch_params(priv, &new_params, preactivate, NULL, true);
+	err = mlx5e_safe_switch_params(priv, &new_params, preactivate, NULL, true, -1);
 	if (err) {
 		if (rl) {
 			mlx5e_mqprio_rl_cleanup(rl);
@@ -3849,7 +3865,7 @@ static int set_feature_lro(struct net_device *netdev, bool enable)
 	}
 
 	err = mlx5e_safe_switch_params(priv, &new_params,
-				       mlx5e_modify_tirs_packet_merge_ctx, NULL, reset);
+				       mlx5e_modify_tirs_packet_merge_ctx, NULL, reset, -1);
 out:
 	mutex_unlock(&priv->state_lock);
 	return err;
@@ -3877,7 +3893,7 @@ static int set_feature_hw_gro(struct net_device *netdev, bool enable)
 		goto out;
 	}
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset, -1);
 out:
 	mutex_unlock(&priv->state_lock);
 	return err;
@@ -3975,7 +3991,7 @@ static int set_feature_rx_fcs(struct net_device *netdev, bool enable)
 	new_params = chs->params;
 	new_params.scatter_fcs_en = enable;
 	err = mlx5e_safe_switch_params(priv, &new_params, mlx5e_set_rx_port_ts_wrap,
-				       &new_params.scatter_fcs_en, true);
+				       &new_params.scatter_fcs_en, true, -1);
 	mutex_unlock(&priv->state_lock);
 	return err;
 }
@@ -4349,7 +4365,7 @@ int mlx5e_change_mtu(struct net_device *netdev, int new_mtu,
 			reset = false;
 	}
 
-	err = mlx5e_safe_switch_params(priv, &new_params, preactivate, NULL, reset);
+	err = mlx5e_safe_switch_params(priv, &new_params, preactivate, NULL, reset, -1);
 
 out:
 	netdev->mtu = params->sw_mtu;
@@ -4400,7 +4416,7 @@ static int mlx5e_hwstamp_config_ptp_rx(struct mlx5e_priv *priv, bool ptp_rx)
 	new_params = priv->channels.params;
 	new_params.ptp_rx = ptp_rx;
 	return mlx5e_safe_switch_params(priv, &new_params, mlx5e_ptp_rx_manage_fs_ctx,
-					&new_params.ptp_rx, true);
+					&new_params.ptp_rx, true, -1);
 }
 
 int mlx5e_hwstamp_set(struct mlx5e_priv *priv, struct ifreq *ifr)
@@ -4831,7 +4847,7 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 
 	old_prog = priv->channels.params.xdp_prog;
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset, -1);
 	if (err)
 		goto unlock;
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
index baa7ef812313..877719c4454a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
@@ -522,7 +522,7 @@ static int mlx5i_change_mtu(struct net_device *netdev, int new_mtu)
 	new_params = priv->channels.params;
 	new_params.sw_mtu = new_mtu;
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true, -1);
 	if (err)
 		goto out;
 
-- 
2.35.1

[net-next RFC v2 3/4] mlx5: Implement mlx5e_ethtool_{get,set}_per_queue_coalesce() to support per-queue operations

[Nabil S. Alramli]

Implment two new methods, mlx5e_ethtool_get_per_queue_coalesce() and
mlx5e_ethtool_set_per_queue_coalesce() and update global coalescing
request handlers to call into them for an extra level of indirection to
allow support for per-queue operations.

Signed-off-by: Nabil S. Alramli <dev@nalramli.com>
---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 152 +++++++++++++-----
 1 file changed, 112 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 13e5838ff1ee..daa0aa833a42 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -513,33 +513,55 @@ static int mlx5e_set_channels(struct net_device *dev,
 	return mlx5e_ethtool_set_channels(priv, ch);
 }
 
-int mlx5e_ethtool_get_coalesce(struct mlx5e_priv *priv,
+static int mlx5e_ethtool_get_per_queue_coalesce(struct mlx5e_priv *priv,
+						int queue,
 			       struct ethtool_coalesce *coal,
 			       struct kernel_ethtool_coalesce *kernel_coal)
 {
 	struct dim_cq_moder *rx_moder, *tx_moder;
+	struct mlx5e_params *params;
 
 	if (!MLX5_CAP_GEN(priv->mdev, cq_moderation))
 		return -EOPNOTSUPP;
 
-	rx_moder = &priv->channels.params.rx_cq_moderation;
+	if (queue != -1 && queue >= priv->channels.num) {
+		netdev_err(priv->netdev, "%s: Invalid queue ID [%d]",
+			   __func__, queue);
+		return -EINVAL;
+	}
+
+	if (queue == -1)
+		params = &priv->channels.params;
+	else
+		params = &priv->channels.c[queue]->params;
+
+	rx_moder = &params->rx_cq_moderation;
 	coal->rx_coalesce_usecs		= rx_moder->usec;
 	coal->rx_max_coalesced_frames	= rx_moder->pkts;
-	coal->use_adaptive_rx_coalesce	= priv->channels.params.rx_dim_enabled;
+	coal->use_adaptive_rx_coalesce	= params->rx_dim_enabled;
 
-	tx_moder = &priv->channels.params.tx_cq_moderation;
+	tx_moder = &params->tx_cq_moderation;
 	coal->tx_coalesce_usecs		= tx_moder->usec;
 	coal->tx_max_coalesced_frames	= tx_moder->pkts;
-	coal->use_adaptive_tx_coalesce	= priv->channels.params.tx_dim_enabled;
+	coal->use_adaptive_tx_coalesce	= params->tx_dim_enabled;
 
-	kernel_coal->use_cqe_mode_rx =
-		MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_RX_CQE_BASED_MODER);
-	kernel_coal->use_cqe_mode_tx =
-		MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_TX_CQE_BASED_MODER);
+	if (kernel_coal) {
+		kernel_coal->use_cqe_mode_rx =
+			MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_RX_CQE_BASED_MODER);
+		kernel_coal->use_cqe_mode_tx =
+			MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_TX_CQE_BASED_MODER);
+	}
 
 	return 0;
 }
 
+int mlx5e_ethtool_get_coalesce(struct mlx5e_priv *priv,
+			       struct ethtool_coalesce *coal,
+			       struct kernel_ethtool_coalesce *kernel_coal)
+{
+	return mlx5e_ethtool_get_per_queue_coalesce(priv, -1, coal, kernel_coal);
+}
+
 static int mlx5e_get_coalesce(struct net_device *netdev,
 			      struct ethtool_coalesce *coal,
 			      struct kernel_ethtool_coalesce *kernel_coal,
@@ -554,32 +576,55 @@ static int mlx5e_get_coalesce(struct net_device *netdev,
 #define MLX5E_MAX_COAL_FRAMES		MLX5_MAX_CQ_COUNT
 
 static void
-mlx5e_set_priv_channels_tx_coalesce(struct mlx5e_priv *priv, struct ethtool_coalesce *coal)
+mlx5e_set_priv_channels_tx_coalesce(struct mlx5e_priv *priv,
+				    int queue,
+				    struct ethtool_coalesce *coal)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	int tc;
 	int i;
 
-	for (i = 0; i < priv->channels.num; ++i) {
-		struct mlx5e_channel *c = priv->channels.c[i];
+	if (queue == -1) {
+		for (i = 0; i < priv->channels.num; ++i) {
+			struct mlx5e_channel *c = priv->channels.c[i];
+
+			for (tc = 0; tc < c->num_tc; tc++) {
+				mlx5_core_modify_cq_moderation(mdev,
+							       &c->sq[tc].cq.mcq,
+							       coal->tx_coalesce_usecs,
+							       coal->tx_max_coalesced_frames);
+			}
+		}
+	} else {
+		struct mlx5e_channel *c = priv->channels.c[queue];
 
 		for (tc = 0; tc < c->num_tc; tc++) {
 			mlx5_core_modify_cq_moderation(mdev,
-						&c->sq[tc].cq.mcq,
-						coal->tx_coalesce_usecs,
-						coal->tx_max_coalesced_frames);
+						       &c->sq[tc].cq.mcq,
+						       coal->tx_coalesce_usecs,
+						       coal->tx_max_coalesced_frames);
 		}
 	}
 }
 
 static void
-mlx5e_set_priv_channels_rx_coalesce(struct mlx5e_priv *priv, struct ethtool_coalesce *coal)
+mlx5e_set_priv_channels_rx_coalesce(struct mlx5e_priv *priv,
+				    int queue,
+				    struct ethtool_coalesce *coal)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	int i;
 
-	for (i = 0; i < priv->channels.num; ++i) {
-		struct mlx5e_channel *c = priv->channels.c[i];
+	if (queue == -1) {
+		for (i = 0; i < priv->channels.num; ++i) {
+			struct mlx5e_channel *c = priv->channels.c[i];
+
+			mlx5_core_modify_cq_moderation(mdev, &c->rq.cq.mcq,
+						       coal->rx_coalesce_usecs,
+						       coal->rx_max_coalesced_frames);
+		}
+	} else {
+		struct mlx5e_channel *c = priv->channels.c[queue];
 
 		mlx5_core_modify_cq_moderation(mdev, &c->rq.cq.mcq,
 					       coal->rx_coalesce_usecs,
@@ -596,15 +641,17 @@ static int cqe_mode_to_period_mode(bool val)
 	return val ? MLX5_CQ_PERIOD_MODE_START_FROM_CQE : MLX5_CQ_PERIOD_MODE_START_FROM_EQE;
 }
 
-int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
+static int mlx5e_ethtool_set_per_queue_coalesce(struct mlx5e_priv *priv,
+						int queue,
 			       struct ethtool_coalesce *coal,
 			       struct kernel_ethtool_coalesce *kernel_coal,
 			       struct netlink_ext_ack *extack)
 {
 	struct dim_cq_moder *rx_moder, *tx_moder;
 	struct mlx5_core_dev *mdev = priv->mdev;
-	struct mlx5e_params new_params;
-	bool reset_rx, reset_tx;
+	bool reset_rx = false, reset_tx = false;
+	struct mlx5e_params new_params = {0};
+	struct mlx5e_params *old_params;
 	bool reset = true;
 	u8 cq_period_mode;
 	int err = 0;
@@ -626,14 +673,29 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 		return -ERANGE;
 	}
 
-	if ((kernel_coal->use_cqe_mode_rx || kernel_coal->use_cqe_mode_tx) &&
-	    !MLX5_CAP_GEN(priv->mdev, cq_period_start_from_cqe)) {
-		NL_SET_ERR_MSG_MOD(extack, "cqe_mode_rx/tx is not supported on this device");
-		return -EOPNOTSUPP;
+	if (kernel_coal) {
+		if ((kernel_coal->use_cqe_mode_rx || kernel_coal->use_cqe_mode_tx) &&
+		    !MLX5_CAP_GEN(priv->mdev, cq_period_start_from_cqe)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "cqe_mode_rx/tx is not supported on this device");
+			return -EOPNOTSUPP;
+		}
+	}
+
+	if (queue != -1 && queue >= priv->channels.num) {
+		netdev_err(priv->netdev, "%s: Invalid queue ID [%d]",
+			   __func__, queue);
+		return -EINVAL;
 	}
 
 	mutex_lock(&priv->state_lock);
-	new_params = priv->channels.params;
+
+	if (queue == -1)
+		old_params = &priv->channels.params;
+	else
+		old_params = &priv->channels.c[queue]->params;
+
+	new_params = *old_params;
 
 	rx_moder          = &new_params.rx_cq_moderation;
 	rx_moder->usec    = coal->rx_coalesce_usecs;
@@ -645,19 +707,21 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 	tx_moder->pkts    = coal->tx_max_coalesced_frames;
 	new_params.tx_dim_enabled = !!coal->use_adaptive_tx_coalesce;
 
-	reset_rx = !!coal->use_adaptive_rx_coalesce != priv->channels.params.rx_dim_enabled;
-	reset_tx = !!coal->use_adaptive_tx_coalesce != priv->channels.params.tx_dim_enabled;
+	reset_rx = !!coal->use_adaptive_rx_coalesce != old_params->rx_dim_enabled;
+	reset_tx = !!coal->use_adaptive_tx_coalesce != old_params->tx_dim_enabled;
 
-	cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_rx);
-	if (cq_period_mode != rx_moder->cq_period_mode) {
-		mlx5e_set_rx_cq_mode_params(&new_params, cq_period_mode);
-		reset_rx = true;
-	}
+	if (kernel_coal) {
+		cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_rx);
+		if (cq_period_mode != rx_moder->cq_period_mode) {
+			mlx5e_set_rx_cq_mode_params(&new_params, cq_period_mode);
+			reset_rx = true;
+		}
 
-	cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_tx);
-	if (cq_period_mode != tx_moder->cq_period_mode) {
-		mlx5e_set_tx_cq_mode_params(&new_params, cq_period_mode);
-		reset_tx = true;
+		cq_period_mode = cqe_mode_to_period_mode(kernel_coal->use_cqe_mode_tx);
+		if (cq_period_mode != tx_moder->cq_period_mode) {
+			mlx5e_set_tx_cq_mode_params(&new_params, cq_period_mode);
+			reset_tx = true;
+		}
 	}
 
 	if (reset_rx) {
@@ -678,18 +742,26 @@ int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
 	 */
 	if (!reset_rx && !reset_tx && test_bit(MLX5E_STATE_OPENED, &priv->state)) {
 		if (!coal->use_adaptive_rx_coalesce)
-			mlx5e_set_priv_channels_rx_coalesce(priv, coal);
+			mlx5e_set_priv_channels_rx_coalesce(priv, queue, coal);
 		if (!coal->use_adaptive_tx_coalesce)
-			mlx5e_set_priv_channels_tx_coalesce(priv, coal);
+			mlx5e_set_priv_channels_tx_coalesce(priv, queue, coal);
 		reset = false;
 	}
 
-	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset, -1);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, reset, queue);
 
 	mutex_unlock(&priv->state_lock);
 	return err;
 }
 
+int mlx5e_ethtool_set_coalesce(struct mlx5e_priv *priv,
+			       struct ethtool_coalesce *coal,
+			       struct kernel_ethtool_coalesce *kernel_coal,
+			       struct netlink_ext_ack *extack)
+{
+	return mlx5e_ethtool_set_per_queue_coalesce(priv, -1, coal, kernel_coal, extack);
+}
+
 static int mlx5e_set_coalesce(struct net_device *netdev,
 			      struct ethtool_coalesce *coal,
 			      struct kernel_ethtool_coalesce *kernel_coal,
-- 
2.35.1

[net-next RFC v2 4/4] mlx5: Add {get,set}_per_queue_coalesce()

[Nabil S. Alramli]

The mlx-5 driver currently only implements querying or modifying coalesce
configurations globally. It does not allow per-queue operations. This
change is to implement per-queue coalesce operations in the driver.

Signed-off-by: Nabil S. Alramli <dev@nalramli.com>
---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 46 +++++++++++++++++--
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index daa0aa833a42..492c03c3d5f4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -515,8 +515,8 @@ static int mlx5e_set_channels(struct net_device *dev,
 
 static int mlx5e_ethtool_get_per_queue_coalesce(struct mlx5e_priv *priv,
 						int queue,
-			       struct ethtool_coalesce *coal,
-			       struct kernel_ethtool_coalesce *kernel_coal)
+						struct ethtool_coalesce *coal,
+						struct kernel_ethtool_coalesce *kernel_coal)
 {
 	struct dim_cq_moder *rx_moder, *tx_moder;
 	struct mlx5e_params *params;
@@ -572,6 +572,23 @@ static int mlx5e_get_coalesce(struct net_device *netdev,
 	return mlx5e_ethtool_get_coalesce(priv, coal, kernel_coal);
 }
 
+/**
+ * mlx5e_get_per_queue_coalesce - gets coalesce settings for particular queue
+ * @netdev: netdev structure
+ * @coal: ethtool's coalesce settings
+ * @queue: the particular queue to read
+ *
+ * Reads a specific queue's coalesce settings
+ **/
+static int mlx5e_get_per_queue_coalesce(struct net_device *netdev,
+					u32 queue,
+					struct ethtool_coalesce *coal)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	return mlx5e_ethtool_get_per_queue_coalesce(priv, queue, coal, NULL);
+}
+
 #define MLX5E_MAX_COAL_TIME		MLX5_MAX_CQ_PERIOD
 #define MLX5E_MAX_COAL_FRAMES		MLX5_MAX_CQ_COUNT
 
@@ -643,9 +660,9 @@ static int cqe_mode_to_period_mode(bool val)
 
 static int mlx5e_ethtool_set_per_queue_coalesce(struct mlx5e_priv *priv,
 						int queue,
-			       struct ethtool_coalesce *coal,
-			       struct kernel_ethtool_coalesce *kernel_coal,
-			       struct netlink_ext_ack *extack)
+						struct ethtool_coalesce *coal,
+						struct kernel_ethtool_coalesce *kernel_coal,
+						struct netlink_ext_ack *extack)
 {
 	struct dim_cq_moder *rx_moder, *tx_moder;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -772,6 +789,23 @@ static int mlx5e_set_coalesce(struct net_device *netdev,
 	return mlx5e_ethtool_set_coalesce(priv, coal, kernel_coal, extack);
 }
 
+/**
+ * mlx5e_set_per_queue_coalesce - set specific queue's coalesce settings
+ * @netdev: the netdev to change
+ * @coal: ethtool's coalesce settings
+ * @queue: the queue to change
+ *
+ * Sets the specified queue's coalesce settings.
+ **/
+static int mlx5e_set_per_queue_coalesce(struct net_device *netdev,
+					u32 queue,
+					struct ethtool_coalesce *coal)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	return mlx5e_ethtool_set_per_queue_coalesce(priv, queue, coal, NULL, NULL);
+}
+
 static void ptys2ethtool_supported_link(struct mlx5_core_dev *mdev,
 					unsigned long *supported_modes,
 					u32 eth_proto_cap)
@@ -2506,6 +2540,8 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.flash_device      = mlx5e_flash_device,
 	.get_priv_flags    = mlx5e_get_priv_flags,
 	.set_priv_flags    = mlx5e_set_priv_flags,
+	.get_per_queue_coalesce	= mlx5e_get_per_queue_coalesce,
+	.set_per_queue_coalesce	= mlx5e_set_per_queue_coalesce,
 	.self_test         = mlx5e_self_test,
 	.get_fec_stats     = mlx5e_get_fec_stats,
 	.get_fecparam      = mlx5e_get_fecparam,
-- 
2.35.1

Re: [net-next RFC v2 0/4] mlx5: support per queue coalesce settings

[Rahul Rameshbabu]

Hi Nabil,

On Mon, 18 Sep, 2023 18:29:51 -0400 "Nabil S. Alramli" <dev@nalramli.com> wrote:
> Hello,
>
> This is v2 of my previous patch:
> https://lore.kernel.org/lkml/20230823223121.58676-1-dev@nalramli.com/.
>
> Saeed: Thanks for reviewing v1. I made significant changes to support
> per-channel DIM settings. Is this ready for an official v1 submission or
> are there other major changes you'd like to see before I do that?
>
> ***************************************************************************
> Version History
> ---------------
> * v1: Initial draft, individual channel DIM changes not supported.
> * v2: Support individual channel DIM changes.
> ***************************************************************************

We actually began working on a patch set for the feature internally
inspired by your initial RFC. If it is alright with you, would it be ok
to have you as a co-author of that series that we should have prepared
in the coming days? We have some minor enhancements that we think will
improve the general architecture for how we handle both the global and
per-queue settings.

>
> Currently, only gobal coalescing configuration queries or changes are
> supported in the `mlx5` driver. However, per-queue operations are not, and
> result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
> adds support for per-queue coalesce operations.
>
> Here's an example use case:
>
> - A mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned to
>   a unique CPU.
> - Two custom RSS contexts are created: context 1 and context 2. Each
>   context has a different set of queues where flows are distributed. For
>   example, context 1 may distribute flows to queues 0-3, and context 2 may
>   distribute flows to queues 4-7.
> - A series of ntuple filters are installed which direct matching flows to
>   RSS contexts. For example, perhaps port 80 is directed to context 1 and
>   port 443 to context 2.
> - Applications which receive network data associated with either context
>   are pinned to the CPUs where the queues in the matching context have
>   their IRQs pinned to maximize locality.
>
> The apps themselves, however, may have different requirements on latency vs
> CPU usage and so setting the per queue IRQ coalesce values would be very
> helpful.
>
> This patch would support this. In v1 DIM mode changes could only be changed
> NIC-wide. However, in this iteration, DIM mode changes are supported
> globally as well as on a per-queue basis.
>
> Here's an example:
>
> ```
> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
> Queue: 2
> Adaptive RX: on  TX: on
> stats-block-usecs: 0
> sample-interval: 0
> pkt-rate-low: 0
> pkt-rate-high: 0
>
> rx-usecs: 8
> rx-frames: 128
> rx-usecs-irq: 0
> rx-frames-irq: 0
>
> tx-usecs: 8
> tx-frames: 128
> tx-usecs-irq: 0
> tx-frames-irq: 0
> ```
>
> Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:
>
> ```
> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx off
> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
> ```
>
> Confirm that the operation succeeded:
>
> ```
> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
> Queue: 2
> Adaptive RX: off  TX: on
> stats-block-usecs: 0
> sample-interval: 0
> pkt-rate-low: 0
> pkt-rate-high: 0
>
> rx-usecs: 16
> rx-frames: 32
> rx-usecs-irq: 0
> rx-frames-irq: 0
>
> tx-usecs: 8
> tx-frames: 128
> tx-usecs-irq: 0
> tx-frames-irq: 0
> ```
>
> The individual channel settings do not overwrite the global ones. However
> Setting the global parameters will also reset all of the individual channel
> options. For example, after we set the options for queue 2, we'll see that
> the global options remain unchanged:
> ```
> $ sudo ethtool --show-coalesce eth0
> Coalesce parameters for eth0:
> Adaptive RX: on  TX: on
> stats-block-usecs: 0
> sample-interval: 0
> pkt-rate-low: 0
> pkt-rate-high: 0
>
> rx-usecs: 8
> rx-frames: 128
> rx-usecs-irq: 0
> rx-frames-irq: 0
>
> tx-usecs: 16
> tx-frames: 32
> tx-usecs-irq: 0
> tx-frames-irq: 0
> ```
>
> But then if we set them, we'll see that the options for queue 2 have been
> reset as well:
> ```
> $ sudo ethtool --coalesce eth0 adaptive-tx off
>
> $ sudo ethtool --show-coalesce eth0
> Coalesce parameters for eth0:
> Adaptive RX: on  TX: off
> stats-block-usecs: 0
> sample-interval: 0
> pkt-rate-low: 0
> pkt-rate-high: 0
>
> rx-usecs: 8
> rx-frames: 128
> rx-usecs-irq: 0
> rx-frames-irq: 0
>
> tx-usecs: 16
> tx-frames: 32
> tx-usecs-irq: 0
> tx-frames-irq: 0
>
> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
> Queue: 2
> Adaptive RX: on  TX: off
> stats-block-usecs: 0
> sample-interval: 0
> pkt-rate-low: 0
> pkt-rate-high: 0
>
> rx-usecs: 8
> rx-frames: 128
> rx-usecs-irq: 0
> rx-frames-irq: 0
>
> tx-usecs: 16
> tx-frames: 32
> tx-usecs-irq: 0
> tx-frames-irq: 0
> ```
>
> Previously a global `struct mlx5e_params` stored the options in
> `struct mlx5e_priv.channels.params`. That was preserved, but a channel-
> specific instance was added as well, in `struct mlx5e_channel.params`.
>
> Best Regards,
>
> ***************************************************************************
>
> Nabil S. Alramli (4):
>   mlx5: Add mlx5e_param to individual mlx5e_channel and preserve them
>     through mlx5e_open_channels()
>   mlx5: Add queue number parameter to mlx5e_safe_switch_params()

We currently are working on a variation of this without needing to use
mlx5e_safe_switch_params for updating individual channel states (our
variation of the feature avoids needing to place an instance of
mlx5e_params per channel).

>   mlx5: Implement mlx5e_ethtool_{get,set}_per_queue_coalesce() to
>     support per-queue operations
>   mlx5: Add {get,set}_per_queue_coalesce()
>
>  drivers/net/ethernet/mellanox/mlx5/core/en.h  |   6 +-
>  .../ethernet/mellanox/mlx5/core/en_dcbnl.c    |   2 +-
>  .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 214 +++++++++++++-----
>  .../net/ethernet/mellanox/mlx5/core/en_main.c |  76 +++++--
>  .../ethernet/mellanox/mlx5/core/ipoib/ipoib.c |   2 +-
>  5 files changed, 222 insertions(+), 78 deletions(-)

--
Thanks,

Rahul Rameshbabu

Re: [net-next RFC v2 0/4] mlx5: support per queue coalesce settings

[Nabil S. Alramli]

Hi Rahul,

Thank you for your response.

On 9/19/23 14:55, Rahul Rameshbabu wrote:
> Hi Nabil,
> 
> On Mon, 18 Sep, 2023 18:29:51 -0400 "Nabil S. Alramli" <dev@nalramli.com> wrote:
>> Hello,
>>
>> This is v2 of my previous patch:
>> https://lore.kernel.org/lkml/20230823223121.58676-1-dev@nalramli.com/.
>>
>> Saeed: Thanks for reviewing v1. I made significant changes to support
>> per-channel DIM settings. Is this ready for an official v1 submission or
>> are there other major changes you'd like to see before I do that?
>>
>> ***************************************************************************
>> Version History
>> ---------------
>> * v1: Initial draft, individual channel DIM changes not supported.
>> * v2: Support individual channel DIM changes.
>> ***************************************************************************
> 
> We actually began working on a patch set for the feature internally
> inspired by your initial RFC. If it is alright with you, would it be ok
> to have you as a co-author of that series that we should have prepared
> in the coming days? We have some minor enhancements that we think will
> improve the general architecture for how we handle both the global and
> per-queue settings.
> 

Yes. Please feel free to add me as a co-author. Actually, I'm new to
submitting mlx-5 patches and a lot of credit goes to Joe Damato
<jdamato@fastly.com> who had this initial idea and helped me develop it
into this patch, so would you mind adding him as well? If you would like
you could start with my patch-set and then revert it and add your own,
or if you think that's too much trouble then I'm fine with however you'd
like to proceed. I'd be happy to test your patch whenever it's ready.

>>
>> Currently, only gobal coalescing configuration queries or changes are
>> supported in the `mlx5` driver. However, per-queue operations are not, and
>> result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
>> adds support for per-queue coalesce operations.
>>
>> Here's an example use case:
>>
>> - A mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned to
>>    a unique CPU.
>> - Two custom RSS contexts are created: context 1 and context 2. Each
>>    context has a different set of queues where flows are distributed. For
>>    example, context 1 may distribute flows to queues 0-3, and context 2 may
>>    distribute flows to queues 4-7.
>> - A series of ntuple filters are installed which direct matching flows to
>>    RSS contexts. For example, perhaps port 80 is directed to context 1 and
>>    port 443 to context 2.
>> - Applications which receive network data associated with either context
>>    are pinned to the CPUs where the queues in the matching context have
>>    their IRQs pinned to maximize locality.
>>
>> The apps themselves, however, may have different requirements on latency vs
>> CPU usage and so setting the per queue IRQ coalesce values would be very
>> helpful.
>>
>> This patch would support this. In v1 DIM mode changes could only be changed
>> NIC-wide. However, in this iteration, DIM mode changes are supported
>> globally as well as on a per-queue basis.
>>
>> Here's an example:
>>
>> ```
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: on  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 8
>> tx-frames: 128
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:
>>
>> ```
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx off
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
>> ```
>>
>> Confirm that the operation succeeded:
>>
>> ```
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: off  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 16
>> rx-frames: 32
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 8
>> tx-frames: 128
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> The individual channel settings do not overwrite the global ones. However
>> Setting the global parameters will also reset all of the individual channel
>> options. For example, after we set the options for queue 2, we'll see that
>> the global options remain unchanged:
>> ```
>> $ sudo ethtool --show-coalesce eth0
>> Coalesce parameters for eth0:
>> Adaptive RX: on  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 16
>> tx-frames: 32
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> But then if we set them, we'll see that the options for queue 2 have been
>> reset as well:
>> ```
>> $ sudo ethtool --coalesce eth0 adaptive-tx off
>>
>> $ sudo ethtool --show-coalesce eth0
>> Coalesce parameters for eth0:
>> Adaptive RX: on  TX: off
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 16
>> tx-frames: 32
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>>
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: on  TX: off
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 16
>> tx-frames: 32
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> Previously a global `struct mlx5e_params` stored the options in
>> `struct mlx5e_priv.channels.params`. That was preserved, but a channel-
>> specific instance was added as well, in `struct mlx5e_channel.params`.
>>
>> Best Regards,
>>
>> ***************************************************************************
>>
>> Nabil S. Alramli (4):
>>    mlx5: Add mlx5e_param to individual mlx5e_channel and preserve them
>>      through mlx5e_open_channels()
>>    mlx5: Add queue number parameter to mlx5e_safe_switch_params()
> 
> We currently are working on a variation of this without needing to use
> mlx5e_safe_switch_params for updating individual channel states (our
> variation of the feature avoids needing to place an instance of
> mlx5e_params per channel).
> 

Oh I'm curious to see how this solution works. I look forward to your
upcoming patch, and would be happy to review it as well.

>>    mlx5: Implement mlx5e_ethtool_{get,set}_per_queue_coalesce() to
>>      support per-queue operations
>>    mlx5: Add {get,set}_per_queue_coalesce()
>>
>>   drivers/net/ethernet/mellanox/mlx5/core/en.h  |   6 +-
>>   .../ethernet/mellanox/mlx5/core/en_dcbnl.c    |   2 +-
>>   .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 214 +++++++++++++-----
>>   .../net/ethernet/mellanox/mlx5/core/en_main.c |  76 +++++--
>>   .../ethernet/mellanox/mlx5/core/ipoib/ipoib.c |   2 +-
>>   5 files changed, 222 insertions(+), 78 deletions(-)
> 
> --
> Thanks,
> 
> Rahul Rameshbabu

Best Regards,

-- Nabil S. Alramli

Re: [net-next RFC v2 0/4] mlx5: support per queue coalesce settings

[Rahul Rameshbabu]

On Tue, 19 Sep, 2023 16:42:27 -0400 "Nabil S. Alramli" <dev@nalramli.com> wrote:
> Hi Rahul,
>
> Thank you for your response.
>
> On 9/19/23 14:55, Rahul Rameshbabu wrote:
>> Hi Nabil,
>> On Mon, 18 Sep, 2023 18:29:51 -0400 "Nabil S. Alramli" <dev@nalramli.com>
>> wrote:
>>> Hello,
>>>
>>> This is v2 of my previous patch:
>>> https://lore.kernel.org/lkml/20230823223121.58676-1-dev@nalramli.com/.
>>>
>>> Saeed: Thanks for reviewing v1. I made significant changes to support
>>> per-channel DIM settings. Is this ready for an official v1 submission or
>>> are there other major changes you'd like to see before I do that?
>>>
>>> ***************************************************************************
>>> Version History
>>> ---------------
>>> * v1: Initial draft, individual channel DIM changes not supported.
>>> * v2: Support individual channel DIM changes.
>>> ***************************************************************************
>> We actually began working on a patch set for the feature internally
>> inspired by your initial RFC. If it is alright with you, would it be ok
>> to have you as a co-author of that series that we should have prepared
>> in the coming days? We have some minor enhancements that we think will
>> improve the general architecture for how we handle both the global and
>> per-queue settings.
>> 
>
> Yes. Please feel free to add me as a co-author. Actually, I'm new to
> submitting mlx-5 patches and a lot of credit goes to Joe Damato
> <jdamato@fastly.com> who had this initial idea and helped me develop it
> into this patch, so would you mind adding him as well? If you would like
> you could start with my patch-set and then revert it and add your own,
> or if you think that's too much trouble then I'm fine with however you'd
> like to proceed. I'd be happy to test your patch whenever it's ready.

Thanks for letting me know. Adding Joe as well as a co-author.

>
>>>
>>> Nabil S. Alramli (4):
>>>    mlx5: Add mlx5e_param to individual mlx5e_channel and preserve them
>>>      through mlx5e_open_channels()
>>>    mlx5: Add queue number parameter to mlx5e_safe_switch_params()
>> We currently are working on a variation of this without needing to use
>> mlx5e_safe_switch_params for updating individual channel states (our
>> variation of the feature avoids needing to place an instance of
>> mlx5e_params per channel).
>> 
>
> Oh I'm curious to see how this solution works. I look forward to your
> upcoming patch, and would be happy to review it as well.

I just wanted to let you know the patch series we are developing is
going through internal review. There are some bits we will need to add
to this series for supporting changing coalescing parameters such as the
cq_period_mode *without* needing to create an entirely new CQ (modifying
the existing CQ).

--
Thanks,

Rahul Rameshbabu