[PATCH] liquidio: make timeout HZ independent and readable

[PATCH] liquidio: make timeout HZ independent and readable

[Nicholas Mc Guire]

schedule_timeout_* takes a timeout in jiffies but the code currently is
passing in a constant which makes this timeout HZ dependent. So define
a constant with (hopefully) meaningful name and pass it through
msecs_to_jiffies() to fix the HZ dependency.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
commit f21fb3ed364b ("Add support of Cavium Liquidio ethernet adapters")
---

Problem found by experimental coccinelle script

The current wait time can vary by a factor 10 depending on the HZ
setting chose, which does not seem reasonable here.

The below patch sets the timeout to 1s - which is the current duration
assuming a setting of HZ== 100. It is though not clear if this is the
intent or if it should be shorter as it is not clear what HZ setting
was assumed during design and used for testing.

This needs an ack by someone who knows the device and can confirm that
waiting 1s for in-flight requests on device removal is reasonable.

Patch was compile tested with: x86_64_defconfig (implies
CONFIG_NET_VENDOR_CAVIUM=y)
(with a large number of sparse warnings though unrelated to the
proposed change)

Patch is against 4.18-rc2 (localversion-next is -next-20180702)

 drivers/net/ethernet/cavium/liquidio/lio_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 7cb4e75..b2d0598 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -91,6 +91,9 @@ static int octeon_console_debug_enabled(u32 console)
  */
 #define LIO_SYNC_OCTEON_TIME_INTERVAL_MS 60000
 
+/* time to wait for possible in-flight requests in milliseconds */
+#define WAIT_INFLIGHT_REQUEST	msecs_to_jiffies(1000)
+
 struct lio_trusted_vf_ctx {
 	struct completion complete;
 	int status;
@@ -259,7 +262,7 @@ static inline void pcierror_quiesce_device(struct octeon_device *oct)
 	force_io_queues_off(oct);
 
 	/* To allow for in-flight requests */
-	schedule_timeout_uninterruptible(100);
+	schedule_timeout_uninterruptible(WAIT_INFLIGHT_REQUEST);
 
 	if (wait_for_pending_requests(oct))
 		dev_err(&oct->pci_dev->dev, "There were pending requests\n");
-- 
2.1.4

RE: [PATCH] liquidio: make timeout HZ independent and readable

[Chickles, Derek]

> From: Nicholas Mc Guire [mailto:hofrat@osadl.org]
> Sent: Monday, July 02, 2018 11:13 AM
> Subject: [PATCH] liquidio: make timeout HZ independent and readable
> 
> schedule_timeout_* takes a timeout in jiffies but the code currently is passing in
> a constant which makes this timeout HZ dependent. So define a constant with
> (hopefully) meaningful name and pass it through
> msecs_to_jiffies() to fix the HZ dependency.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> commit f21fb3ed364b
> ("Add support of Cavium Liquidio ethernet adapters")
> ---
> 
> Problem found by experimental coccinelle script
> 
> The current wait time can vary by a factor 10 depending on the HZ setting chose,
> which does not seem reasonable here.
> 
> The below patch sets the timeout to 1s - which is the current duration assuming
> a setting of HZ== 100. It is though not clear if this is the intent or if it should be
> shorter as it is not clear what HZ setting was assumed during design and used for
> testing.
> 
> This needs an ack by someone who knows the device and can confirm that
> waiting 1s for in-flight requests on device removal is reasonable.
> 
> Patch was compile tested with: x86_64_defconfig (implies
> CONFIG_NET_VENDOR_CAVIUM=y)
> (with a large number of sparse warnings though unrelated to the proposed
> change)
> 
> Patch is against 4.18-rc2 (localversion-next is -next-20180702)
> 
>  drivers/net/ethernet/cavium/liquidio/lio_main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c
> b/drivers/net/ethernet/cavium/liquidio/lio_main.c
> index 7cb4e75..b2d0598 100644
> --- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
> @@ -91,6 +91,9 @@ static int octeon_console_debug_enabled(u32 console)
>   */
>  #define LIO_SYNC_OCTEON_TIME_INTERVAL_MS 60000
> 
> +/* time to wait for possible in-flight requests in milliseconds */
> +#define WAIT_INFLIGHT_REQUEST  msecs_to_jiffies(1000)
> +
>  struct lio_trusted_vf_ctx {
>         struct completion complete;
>         int status;
> @@ -259,7 +262,7 @@ static inline void pcierror_quiesce_device(struct
> octeon_device *oct)
>         force_io_queues_off(oct);
> 
>         /* To allow for in-flight requests */
> -       schedule_timeout_uninterruptible(100);
> +       schedule_timeout_uninterruptible(WAIT_INFLIGHT_REQUEST);
> 
>         if (wait_for_pending_requests(oct))
>                 dev_err(&oct->pci_dev->dev, "There were pending requests\n");
> --
> 2.1.4

Looks fine.

Acked-by: Derek Chickles <derek.chickles@caviumnetworks.com>

Re: [PATCH] liquidio: make timeout HZ independent and readable

[David Miller]

From: Nicholas Mc Guire <hofrat@osadl.org>
Date: Mon,  2 Jul 2018 20:13:15 +0200

> schedule_timeout_* takes a timeout in jiffies but the code currently is
> passing in a constant which makes this timeout HZ dependent. So define
> a constant with (hopefully) meaningful name and pass it through
> msecs_to_jiffies() to fix the HZ dependency.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> commit f21fb3ed364b ("Add support of Cavium Liquidio ethernet adapters")

Applied, thank you.