[Intel-wired-lan] [PATCH iwl-next] ice: reduce initial wait for control queue messages

[Intel-wired-lan] [PATCH iwl-next] ice: reduce initial wait for control queue messages

[Jacob Keller]

The ice_sq_send_cmd() function is used to send messages to the control
queues used to communicate with firmware, virtual functions, and even some
hardware.

When sending a control queue message, the driver is designed to
synchronously wait for a response from the queue. Currently it waits
between checks for 100 to 150 microseconds.

Commit f86d6f9c49f6 ("ice: sleep, don't busy-wait, for
ICE_CTL_Q_SQ_CMD_TIMEOUT") did recently change the behavior from an
unnecessary delay into a sleep which is a significant improvement over the
old behavior of polling using udelay.

Because of the nature of PCIe transactions, the hardware won't be informed
about a new message until the write to the tail register posts. This is
only guaranteed to occur at the next register read. In ice_sq_send_cmd(),
this happens at the ice_sq_done() call. Because of this, the driver
essentially forces a minimum of one full wait time regardless of how fast
the response is.

For the hardware-based sideband queue, this is especially slow. It is
expected that the hardware will respond within 2 or 3 microseconds, an
order of magnitude faster than the 100-150 microsecond sleep.

Allow such fast completions to occur without delay by introducing a small 5
microsecond delay first before entering the sleeping timeout loop. Ensure
the tail write has been posted by using ice_flush(hw) first.

While at it, lets also remove the ICE_CTL_Q_SQ_CMD_USEC macro as it
obscures the sleep time in the inner loop. It was likely introduced to
avoid "magic numbers", but in practice sleep and delay values are easier to
read and understand when using actual numbers instead of a named constant.

This change should allow the fast hardware based control queue messages to
complete quickly without delay, while slower firmware queue response times
will sleep while waiting for the response.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---

@Michal, do you think this initial 5 microsecond delay would have
significant impact on the use cases that you fixed in the mentioned commit?
I don't want to break those, but do want to make sure that the fast response
hardware queues don't wait unnecessarily. One alternative I considered was
only doing this delay if we are on the appropriate queue type. I'd
appreciate your thoughts.

 drivers/net/ethernet/intel/ice/ice_controlq.c | 9 +++++++--
 drivers/net/ethernet/intel/ice/ice_controlq.h | 1 -
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index d2faf1baad2f..385fd88831db 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -1056,14 +1056,19 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	if (cq->sq.next_to_use == cq->sq.count)
 		cq->sq.next_to_use = 0;
 	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
+	ice_flush(hw);
+
+	/* Wait a short time before initial ice_sq_done() check, to allow
+	 * hardware time for completion.
+	 */
+	udelay(5);
 
 	timeout = jiffies + ICE_CTL_Q_SQ_CMD_TIMEOUT;
 	do {
 		if (ice_sq_done(hw, cq))
 			break;
 
-		usleep_range(ICE_CTL_Q_SQ_CMD_USEC,
-			     ICE_CTL_Q_SQ_CMD_USEC * 3 / 2);
+		usleep_range(100, 150);
 	} while (time_before(jiffies, timeout));
 
 	/* if ready, copy the desc back to temp */
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
index 950b7f4a7a05..8f2fd1613a95 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
@@ -35,7 +35,6 @@ enum ice_ctl_q {
 
 /* Control Queue timeout settings - max delay 1s */
 #define ICE_CTL_Q_SQ_CMD_TIMEOUT	HZ    /* Wait max 1s */
-#define ICE_CTL_Q_SQ_CMD_USEC		100   /* Check every 100usec */
 #define ICE_CTL_Q_ADMIN_INIT_TIMEOUT	10    /* Count 10 times */
 #define ICE_CTL_Q_ADMIN_INIT_MSEC	100   /* Check every 100msec */
 
-- 
2.40.0.471.gbd7f14d9353b

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next] ice: reduce initial wait for control queue messages

[Michal Schmidt]

On Tue, Jun 13, 2023 at 10:41 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> The ice_sq_send_cmd() function is used to send messages to the control
> queues used to communicate with firmware, virtual functions, and even some
> hardware.
>
> When sending a control queue message, the driver is designed to
> synchronously wait for a response from the queue. Currently it waits
> between checks for 100 to 150 microseconds.
>
> Commit f86d6f9c49f6 ("ice: sleep, don't busy-wait, for
> ICE_CTL_Q_SQ_CMD_TIMEOUT") did recently change the behavior from an
> unnecessary delay into a sleep which is a significant improvement over the
> old behavior of polling using udelay.
>
> Because of the nature of PCIe transactions, the hardware won't be informed
> about a new message until the write to the tail register posts. This is
> only guaranteed to occur at the next register read. In ice_sq_send_cmd(),
> this happens at the ice_sq_done() call. Because of this, the driver
> essentially forces a minimum of one full wait time regardless of how fast
> the response is.
>
> For the hardware-based sideband queue, this is especially slow. It is
> expected that the hardware will respond within 2 or 3 microseconds, an
> order of magnitude faster than the 100-150 microsecond sleep.
>
> Allow such fast completions to occur without delay by introducing a small 5
> microsecond delay first before entering the sleeping timeout loop. Ensure
> the tail write has been posted by using ice_flush(hw) first.
>
> While at it, lets also remove the ICE_CTL_Q_SQ_CMD_USEC macro as it
> obscures the sleep time in the inner loop. It was likely introduced to
> avoid "magic numbers", but in practice sleep and delay values are easier to
> read and understand when using actual numbers instead of a named constant.
>
> This change should allow the fast hardware based control queue messages to
> complete quickly without delay, while slower firmware queue response times
> will sleep while waiting for the response.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>
> @Michal, do you think this initial 5 microsecond delay would have
> significant impact on the use cases that you fixed in the mentioned commit?
> I don't want to break those, but do want to make sure that the fast response
> hardware queues don't wait unnecessarily. One alternative I considered was
> only doing this delay if we are on the appropriate queue type. I'd
> appreciate your thoughts.

I think 5 us is quick enough that this won't be a problem.

Reviewed-by: Michal Schmidt <mschmidt@redhat.com>

>  drivers/net/ethernet/intel/ice/ice_controlq.c | 9 +++++++--
>  drivers/net/ethernet/intel/ice/ice_controlq.h | 1 -
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
> index d2faf1baad2f..385fd88831db 100644
> --- a/drivers/net/ethernet/intel/ice/ice_controlq.c
> +++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
> @@ -1056,14 +1056,19 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
>         if (cq->sq.next_to_use == cq->sq.count)
>                 cq->sq.next_to_use = 0;
>         wr32(hw, cq->sq.tail, cq->sq.next_to_use);
> +       ice_flush(hw);
> +
> +       /* Wait a short time before initial ice_sq_done() check, to allow
> +        * hardware time for completion.
> +        */
> +       udelay(5);
>
>         timeout = jiffies + ICE_CTL_Q_SQ_CMD_TIMEOUT;
>         do {
>                 if (ice_sq_done(hw, cq))
>                         break;
>
> -               usleep_range(ICE_CTL_Q_SQ_CMD_USEC,
> -                            ICE_CTL_Q_SQ_CMD_USEC * 3 / 2);
> +               usleep_range(100, 150);
>         } while (time_before(jiffies, timeout));
>
>         /* if ready, copy the desc back to temp */
> diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
> index 950b7f4a7a05..8f2fd1613a95 100644
> --- a/drivers/net/ethernet/intel/ice/ice_controlq.h
> +++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
> @@ -35,7 +35,6 @@ enum ice_ctl_q {
>
>  /* Control Queue timeout settings - max delay 1s */
>  #define ICE_CTL_Q_SQ_CMD_TIMEOUT       HZ    /* Wait max 1s */
> -#define ICE_CTL_Q_SQ_CMD_USEC          100   /* Check every 100usec */
>  #define ICE_CTL_Q_ADMIN_INIT_TIMEOUT   10    /* Count 10 times */
>  #define ICE_CTL_Q_ADMIN_INIT_MSEC      100   /* Check every 100msec */
>
> --
> 2.40.0.471.gbd7f14d9353b
>

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next] ice: reduce initial wait for control queue messages

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob Keller
> Sent: Wednesday, June 14, 2023 2:11 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; mschmidt <mschmidt@redhat.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next] ice: reduce initial wait for control queue messages
>
> The ice_sq_send_cmd() function is used to send messages to the control queues used to communicate with firmware, virtual functions, and even some hardware.
>
> When sending a control queue message, the driver is designed to synchronously wait for a response from the queue. Currently it waits between checks for 100 to 150 microseconds.
>
> Commit f86d6f9c49f6 ("ice: sleep, don't busy-wait, for
> ICE_CTL_Q_SQ_CMD_TIMEOUT") did recently change the behavior from an unnecessary delay into a sleep which is a significant improvement over the old behavior of polling using udelay.
>
> Because of the nature of PCIe transactions, the hardware won't be informed about a new message until the write to the tail register posts. This is only guaranteed to occur at the next register read. In ice_sq_send_cmd(), this happens at the ice_sq_done() call. Because of this, the driver essentially forces a minimum of one full wait time regardless of how fast the response is.
> 
> For the hardware-based sideband queue, this is especially slow. It is expected that the hardware will respond within 2 or 3 microseconds, an order of magnitude faster than the 100-150 microsecond sleep.
>
> Allow such fast completions to occur without delay by introducing a small 5 microsecond delay first before entering the sleeping timeout loop. Ensure the tail write has been posted by using ice_flush(hw) first.
>
> While at it, lets also remove the ICE_CTL_Q_SQ_CMD_USEC macro as it obscures the sleep time in the inner loop. It was likely introduced to avoid "magic numbers", but in practice sleep and delay values are easier to read and understand when using actual numbers instead of a named constant.
>
> This change should allow the fast hardware based control queue messages to complete quickly without delay, while slower firmware queue response times will sleep while waiting for the response.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>
> @Michal, do you think this initial 5 microsecond delay would have significant impact on the use cases that you fixed in the mentioned commit?
> I don't want to break those, but do want to make sure that the fast response hardware queues don't wait unnecessarily. One alternative I considered was only doing this delay if we are on the appropriate queue type. I'd appreciate your thoughts.
>
> drivers/net/ethernet/intel/ice/ice_controlq.c | 9 +++++++--  drivers/net/ethernet/intel/ice/ice_controlq.h | 1 -
> 2 files changed, 7 insertions(+), 3 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan