[dpdk-dev] [PATCH] net/sfc: do not panic if alarms are not supported

[dpdk-dev] [PATCH] net/sfc: do not panic if alarms are not supported

[Andrew Rybchenko]

Alarms are not supported on the FreeBSD.
Application must poll link status periodically itself using
rte_eth_link_get_nowait() to avoid managment event queue overflow.

Fixes: 2de39f4e1310 ("net/sfc: periodic management EVQ polling using alarm")

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Reviewed-by: Andrew Lee <alee@solarflare.com>
Reviewed-by: Andy Moreton <amoreton@solarflare.com>
---
 drivers/net/sfc/sfc_ev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index c788986..fe6de6f 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -499,10 +499,14 @@
 
 	rc = rte_eal_alarm_set(SFC_MGMT_EV_QPOLL_PERIOD_US,
 			       sfc_ev_mgmt_periodic_qpoll, sa);
-	if (rc != 0)
-		sfc_panic(sa,
-			  "cannot rearm management EVQ polling alarm (rc=%d)",
-			  rc);
+	if (rc == -ENOTSUP) {
+		sfc_warn(sa, "alarms are not supported");
+		sfc_warn(sa, "management EVQ must be polled indirectly using no-wait link status update");
+	} else if (rc != 0) {
+		sfc_err(sa,
+			"cannot rearm management EVQ polling alarm (rc=%d)",
+			rc);
+	}
 }
 
 static void
-- 
1.8.2.3

[dpdk-dev] [PATCH v2] net/sfc: do not panic if alarms are not supported

[Andrew Rybchenko]

Alarms are not supported on the FreeBSD.
Application must poll link status periodically itself using
rte_eth_link_get_nowait() to avoid management event queue overflow.

Fixes: 2de39f4e1310 ("net/sfc: periodic management EVQ polling using alarm")

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Reviewed-by: Andrew Lee <alee@solarflare.com>
Reviewed-by: Andy Moreton <amoreton@solarflare.com>
---
v2:
 - fix spelling

 drivers/net/sfc/sfc_ev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index c788986..fe6de6f 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -499,10 +499,14 @@
 
 	rc = rte_eal_alarm_set(SFC_MGMT_EV_QPOLL_PERIOD_US,
 			       sfc_ev_mgmt_periodic_qpoll, sa);
-	if (rc != 0)
-		sfc_panic(sa,
-			  "cannot rearm management EVQ polling alarm (rc=%d)",
-			  rc);
+	if (rc == -ENOTSUP) {
+		sfc_warn(sa, "alarms are not supported");
+		sfc_warn(sa, "management EVQ must be polled indirectly using no-wait link status update");
+	} else if (rc != 0) {
+		sfc_err(sa,
+			"cannot rearm management EVQ polling alarm (rc=%d)",
+			rc);
+	}
 }
 
 static void
-- 
1.8.2.3

Re: [dpdk-dev] [PATCH v2] net/sfc: do not panic if alarms are not supported

[Ferruh Yigit]

On 1/19/2017 11:12 AM, Andrew Rybchenko wrote:
> Alarms are not supported on the FreeBSD.
> Application must poll link status periodically itself using
> rte_eth_link_get_nowait() to avoid management event queue overflow.
> 
> Fixes: 2de39f4e1310 ("net/sfc: periodic management EVQ polling using alarm")
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Reviewed-by: Andrew Lee <alee@solarflare.com>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> ---
> v2:
>  - fix spelling
> 
>  drivers/net/sfc/sfc_ev.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
> index c788986..fe6de6f 100644
> --- a/drivers/net/sfc/sfc_ev.c
> +++ b/drivers/net/sfc/sfc_ev.c
> @@ -499,10 +499,14 @@
>  
>  	rc = rte_eal_alarm_set(SFC_MGMT_EV_QPOLL_PERIOD_US,
>  			       sfc_ev_mgmt_periodic_qpoll, sa);
> -	if (rc != 0)
> -		sfc_panic(sa,
> -			  "cannot rearm management EVQ polling alarm (rc=%d)",
> -			  rc);
> +	if (rc == -ENOTSUP) {
> +		sfc_warn(sa, "alarms are not supported");
> +		sfc_warn(sa, "management EVQ must be polled indirectly using no-wait link status update");

Who is the audience of this message, I am just trying to understand.

If this is for application developer, perhaps function should return and
error and log should be a debug log.

Or if it is for end user of the application, and this issue is something
that prevents app run properly, perhaps application should return error
instead of logging warning.

Overall I am a little suspicious about warn/err level of logs that does
not alter the execution path. I would like to hear more comments indeed.


> +	} else if (rc != 0) {
> +		sfc_err(sa,
> +			"cannot rearm management EVQ polling alarm (rc=%d)",
> +			rc);
> +	}
>  }
>  
>  static void
> 

Re: [dpdk-dev] [PATCH v2] net/sfc: do not panic if alarms are not supported

[Andrew Rybchenko]

On 01/20/2017 03:36 PM, Ferruh Yigit wrote:
> On 1/19/2017 11:12 AM, Andrew Rybchenko wrote:
>> Alarms are not supported on the FreeBSD.
>> Application must poll link status periodically itself using
>> rte_eth_link_get_nowait() to avoid management event queue overflow.
>>
>> Fixes: 2de39f4e1310 ("net/sfc: periodic management EVQ polling using alarm")
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> Reviewed-by: Andrew Lee <alee@solarflare.com>
>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> ---
>> v2:
>>   - fix spelling
>>
>>   drivers/net/sfc/sfc_ev.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
>> index c788986..fe6de6f 100644
>> --- a/drivers/net/sfc/sfc_ev.c
>> +++ b/drivers/net/sfc/sfc_ev.c
>> @@ -499,10 +499,14 @@
>>   
>>   	rc = rte_eal_alarm_set(SFC_MGMT_EV_QPOLL_PERIOD_US,
>>   			       sfc_ev_mgmt_periodic_qpoll, sa);
>> -	if (rc != 0)
>> -		sfc_panic(sa,
>> -			  "cannot rearm management EVQ polling alarm (rc=%d)",
>> -			  rc);
>> +	if (rc == -ENOTSUP) {
>> +		sfc_warn(sa, "alarms are not supported");
>> +		sfc_warn(sa, "management EVQ must be polled indirectly using no-wait link status update");
> Who is the audience of this message, I am just trying to understand.

DPDK application developer

> If this is for application developer, perhaps function should return and
> error and log should be a debug log.

Unfortunately there is no way to know if application will poll the link 
status with no-wait.
So, we cannot distinguish when it is OK to continue (yes alarms are not 
supported, but
application is aware) and when is it not OK (alarms are not supported 
and application is
unaware).

> Or if it is for end user of the application, and this issue is something
> that prevents app run properly, perhaps application should return error
> instead of logging warning.
>
> Overall I am a little suspicious about warn/err level of logs that does
> not alter the execution path. I would like to hear more comments indeed.
>
>
>> +	} else if (rc != 0) {
>> +		sfc_err(sa,
>> +			"cannot rearm management EVQ polling alarm (rc=%d)",
>> +			rc);
>> +	}
>>   }
>>   
>>   static void
>>
>>

Re: [dpdk-dev] [PATCH v2] net/sfc: do not panic if alarms are not supported

[Ferruh Yigit]

On 1/19/2017 11:12 AM, Andrew Rybchenko wrote:
> Alarms are not supported on the FreeBSD.
> Application must poll link status periodically itself using
> rte_eth_link_get_nowait() to avoid management event queue overflow.
> 
> Fixes: 2de39f4e1310 ("net/sfc: periodic management EVQ polling using alarm")
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Reviewed-by: Andrew Lee <alee@solarflare.com>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>

Applied to dpdk-next-net/master, thanks.