* [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request
@ 2021-09-03 6:48 Jedrzej Jagielski
2021-09-03 7:43 ` Paul Menzel
2021-09-09 20:38 ` Nguyen, Anthony L
0 siblings, 2 replies; 5+ messages in thread
From: Jedrzej Jagielski @ 2021-09-03 6:48 UTC (permalink / raw)
To: intel-wired-lan
Currently iavf adapter statistics are refreshed only in a
watchdog task, triggered approximately every two seconds,
which causes some ethtool requests to return outdated values.
Add explicit statistics refresh when requested by ethtool -S.
Fixes: b476b0030e61 ("iavf: Move commands processing to the separate function")
Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
drivers/net/ethernet/intel/iavf/iavf.h | 2 ++
drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +++
drivers/net/ethernet/intel/iavf/iavf_main.c | 18 ++++++++++++++++++
3 files changed, 23 insertions(+)
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 21c95775509a..afe6b0d24a9a 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -306,6 +306,7 @@ struct iavf_adapter {
#define IAVF_FLAG_AQ_DEL_FDIR_FILTER BIT(26)
#define IAVF_FLAG_AQ_ADD_ADV_RSS_CFG BIT(27)
#define IAVF_FLAG_AQ_DEL_ADV_RSS_CFG BIT(28)
+#define IAVF_FLAG_AQ_REQUEST_STATS BIT(29)
/* OS defined structs */
struct net_device *netdev;
@@ -399,6 +400,7 @@ int iavf_up(struct iavf_adapter *adapter);
void iavf_down(struct iavf_adapter *adapter);
int iavf_process_config(struct iavf_adapter *adapter);
void iavf_schedule_reset(struct iavf_adapter *adapter);
+void iavf_schedule_request_stats(struct iavf_adapter *adapter);
void iavf_reset(struct iavf_adapter *adapter);
void iavf_set_ethtool_ops(struct net_device *netdev);
void iavf_update_stats(struct iavf_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index 7cbe59beeebb..21c4d4180f3e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -354,6 +354,9 @@ static void iavf_get_ethtool_stats(struct net_device *netdev,
struct iavf_adapter *adapter = netdev_priv(netdev);
unsigned int i;
+ /* Explicitly request stats refresh */
+ iavf_schedule_request_stats(adapter);
+
iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
rcu_read_lock();
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 80437ef26391..e7ac6356772b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -165,6 +165,19 @@ void iavf_schedule_reset(struct iavf_adapter *adapter)
}
}
+/**
+ * iavf_schedule_request_stats - Set the flags and schedule statistics request
+ * @adapter: board private structure
+ *
+ * Sets IAVF_FLAG_AQ_REQUEST_STATS flag so iavf_watchdog_task() will explicitly
+ * request and refresh ethtool stats
+ **/
+void iavf_schedule_request_stats(struct iavf_adapter *adapter)
+{
+ adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_STATS;
+ queue_work(iavf_wq, &adapter->watchdog_task.work);
+}
+
/**
* iavf_tx_timeout - Respond to a Tx Hang
* @netdev: network interface device structure
@@ -1700,6 +1713,11 @@ static int iavf_process_aq_command(struct iavf_adapter *adapter)
iavf_del_adv_rss_cfg(adapter);
return 0;
}
+ if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_STATS) {
+ iavf_request_stats(adapter);
+ return IAVF_SUCCESS;
+ }
+
return -EAGAIN;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request
2021-09-03 6:48 [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request Jedrzej Jagielski
@ 2021-09-03 7:43 ` Paul Menzel
2021-09-07 12:54 ` Jagielski, Jedrzej
2021-09-09 20:38 ` Nguyen, Anthony L
1 sibling, 1 reply; 5+ messages in thread
From: Paul Menzel @ 2021-09-03 7:43 UTC (permalink / raw)
To: intel-wired-lan
Dear Jedrzej,
Am 03.09.21 um 08:48 schrieb Jedrzej Jagielski:
Should the summary have a net prefix or so? Also shorter:
> iavf: Refresh iavf adapter stats on ethtool request
> Currently iavf adapter statistics are refreshed only in a
> watchdog task, triggered approximately every two seconds,
> which causes some ethtool requests to return outdated values.
>
> Add explicit statistics refresh when requested by ethtool -S.
>
> Fixes: b476b0030e61 ("iavf: Move commands processing to the separate function")
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 2 ++
> drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +++
> drivers/net/ethernet/intel/iavf/iavf_main.c | 18 ++++++++++++++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index 21c95775509a..afe6b0d24a9a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -306,6 +306,7 @@ struct iavf_adapter {
> #define IAVF_FLAG_AQ_DEL_FDIR_FILTER BIT(26)
> #define IAVF_FLAG_AQ_ADD_ADV_RSS_CFG BIT(27)
> #define IAVF_FLAG_AQ_DEL_ADV_RSS_CFG BIT(28)
> +#define IAVF_FLAG_AQ_REQUEST_STATS BIT(29)
>
> /* OS defined structs */
> struct net_device *netdev;
> @@ -399,6 +400,7 @@ int iavf_up(struct iavf_adapter *adapter);
> void iavf_down(struct iavf_adapter *adapter);
> int iavf_process_config(struct iavf_adapter *adapter);
> void iavf_schedule_reset(struct iavf_adapter *adapter);
> +void iavf_schedule_request_stats(struct iavf_adapter *adapter);
> void iavf_reset(struct iavf_adapter *adapter);
> void iavf_set_ethtool_ops(struct net_device *netdev);
> void iavf_update_stats(struct iavf_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 7cbe59beeebb..21c4d4180f3e 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -354,6 +354,9 @@ static void iavf_get_ethtool_stats(struct net_device *netdev,
> struct iavf_adapter *adapter = netdev_priv(netdev);
> unsigned int i;
>
> + /* Explicitly request stats refresh */
> + iavf_schedule_request_stats(adapter);
> +
> iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
>
> rcu_read_lock();
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 80437ef26391..e7ac6356772b 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -165,6 +165,19 @@ void iavf_schedule_reset(struct iavf_adapter *adapter)
> }
> }
>
> +/**
> + * iavf_schedule_request_stats - Set the flags and schedule statistics request
> + * @adapter: board private structure
> + *
> + * Sets IAVF_FLAG_AQ_REQUEST_STATS flag so iavf_watchdog_task() will explicitly
> + * request and refresh ethtool stats
> + **/
> +void iavf_schedule_request_stats(struct iavf_adapter *adapter)
> +{
> + adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_STATS;
> + queue_work(iavf_wq, &adapter->watchdog_task.work);
> +}
> +
> /**
> * iavf_tx_timeout - Respond to a Tx Hang
> * @netdev: network interface device structure
> @@ -1700,6 +1713,11 @@ static int iavf_process_aq_command(struct iavf_adapter *adapter)
> iavf_del_adv_rss_cfg(adapter);
> return 0;
> }
> + if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_STATS) {
> + iavf_request_stats(adapter);
Should that be `iavf_schedule_request_stats()`?
> + return IAVF_SUCCESS;
> + }
> +
> return -EAGAIN;
> }
Kind regards,
Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request
2021-09-03 7:43 ` Paul Menzel
@ 2021-09-07 12:54 ` Jagielski, Jedrzej
2021-09-07 14:36 ` Paul Menzel
0 siblings, 1 reply; 5+ messages in thread
From: Jagielski, Jedrzej @ 2021-09-07 12:54 UTC (permalink / raw)
To: intel-wired-lan
Dear Paul,
" Should the summary have a net prefix or so? Also shorter:
> iavf: Refresh iavf adapter stats on ethtool request"
Shouldn't the commit title begin begins with one from the words <fix/add/del/refactor>?
"Should that be `iavf_schedule_request_stats()`?"
This function is invoked only from iavf_get_ethtool_stats.
Best regards,
J?drzej
-----Original Message-----
From: Paul Menzel <pmenzel@molgen.mpg.de>
Sent: pi?tek, 3 wrze?nia 2021 09:43
To: Jagielski, Jedrzej <jedrzej.jagielski@intel.com>
Cc: intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request
Dear Jedrzej,
Am 03.09.21 um 08:48 schrieb Jedrzej Jagielski:
Should the summary have a net prefix or so? Also shorter:
> iavf: Refresh iavf adapter stats on ethtool request
> Currently iavf adapter statistics are refreshed only in a watchdog
> task, triggered approximately every two seconds, which causes some
> ethtool requests to return outdated values.
>
> Add explicit statistics refresh when requested by ethtool -S.
>
> Fixes: b476b0030e61 ("iavf: Move commands processing to the separate
> function")
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 2 ++
> drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +++
> drivers/net/ethernet/intel/iavf/iavf_main.c | 18 ++++++++++++++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index 21c95775509a..afe6b0d24a9a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -306,6 +306,7 @@ struct iavf_adapter {
> #define IAVF_FLAG_AQ_DEL_FDIR_FILTER BIT(26)
> #define IAVF_FLAG_AQ_ADD_ADV_RSS_CFG BIT(27)
> #define IAVF_FLAG_AQ_DEL_ADV_RSS_CFG BIT(28)
> +#define IAVF_FLAG_AQ_REQUEST_STATS BIT(29)
>
> /* OS defined structs */
> struct net_device *netdev;
> @@ -399,6 +400,7 @@ int iavf_up(struct iavf_adapter *adapter);
> void iavf_down(struct iavf_adapter *adapter);
> int iavf_process_config(struct iavf_adapter *adapter);
> void iavf_schedule_reset(struct iavf_adapter *adapter);
> +void iavf_schedule_request_stats(struct iavf_adapter *adapter);
> void iavf_reset(struct iavf_adapter *adapter);
> void iavf_set_ethtool_ops(struct net_device *netdev);
> void iavf_update_stats(struct iavf_adapter *adapter); diff --git
> a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 7cbe59beeebb..21c4d4180f3e 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -354,6 +354,9 @@ static void iavf_get_ethtool_stats(struct net_device *netdev,
> struct iavf_adapter *adapter = netdev_priv(netdev);
> unsigned int i;
>
> + /* Explicitly request stats refresh */
> + iavf_schedule_request_stats(adapter);
> +
> iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
>
> rcu_read_lock();
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 80437ef26391..e7ac6356772b 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -165,6 +165,19 @@ void iavf_schedule_reset(struct iavf_adapter *adapter)
> }
> }
>
> +/**
> + * iavf_schedule_request_stats - Set the flags and schedule
> +statistics request
> + * @adapter: board private structure
> + *
> + * Sets IAVF_FLAG_AQ_REQUEST_STATS flag so iavf_watchdog_task() will
> +explicitly
> + * request and refresh ethtool stats
> + **/
> +void iavf_schedule_request_stats(struct iavf_adapter *adapter) {
> + adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_STATS;
> + queue_work(iavf_wq, &adapter->watchdog_task.work); }
> +
> /**
> * iavf_tx_timeout - Respond to a Tx Hang
> * @netdev: network interface device structure @@ -1700,6 +1713,11
> @@ static int iavf_process_aq_command(struct iavf_adapter *adapter)
> iavf_del_adv_rss_cfg(adapter);
> return 0;
> }
> + if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_STATS) {
> + iavf_request_stats(adapter);
Should that be `iavf_schedule_request_stats()`?
> + return IAVF_SUCCESS;
> + }
> +
> return -EAGAIN;
> }
Kind regards,
Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request
2021-09-07 12:54 ` Jagielski, Jedrzej
@ 2021-09-07 14:36 ` Paul Menzel
0 siblings, 0 replies; 5+ messages in thread
From: Paul Menzel @ 2021-09-07 14:36 UTC (permalink / raw)
To: intel-wired-lan
Dear J?drzej,
Am 07.09.21 um 14:54 schrieb Jagielski, Jedrzej:
> "Should the summary have a net prefix or so? Also shorter:
>
>> iavf: Refresh iavf adapter stats on ethtool request"
>
> Shouldn't the commit title begin begins with one from the words <fix/add/del/refactor>?
I am not aware of such a rule.
> "Should that be `iavf_schedule_request_stats()`?"
> This function is invoked only from iavf_get_ethtool_stats.
Reading the diff again, I am not sure how I missed that. Sorry for the
noise.
Kind regards,
Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request
2021-09-03 6:48 [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request Jedrzej Jagielski
2021-09-03 7:43 ` Paul Menzel
@ 2021-09-09 20:38 ` Nguyen, Anthony L
1 sibling, 0 replies; 5+ messages in thread
From: Nguyen, Anthony L @ 2021-09-09 20:38 UTC (permalink / raw)
To: intel-wired-lan
On Fri, 2021-09-03 at 06:48 +0000, Jedrzej Jagielski wrote:
> Currently iavf adapter statistics are refreshed only in a
> watchdog task, triggered approximately every two seconds,
> which causes some ethtool requests to return outdated values.
>
> Add explicit statistics refresh when requested by ethtool -S.
>
> Fixes: b476b0030e61 ("iavf: Move commands processing to the separate
> function")
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 2 ++
> drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 3 +++
> drivers/net/ethernet/intel/iavf/iavf_main.c | 18
> ++++++++++++++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index 21c95775509a..afe6b0d24a9a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -306,6 +306,7 @@ struct iavf_adapter {
> #define IAVF_FLAG_AQ_DEL_FDIR_FILTER BIT(26)
> #define IAVF_FLAG_AQ_ADD_ADV_RSS_CFG BIT(27)
> #define IAVF_FLAG_AQ_DEL_ADV_RSS_CFG BIT(28)
> +#define IAVF_FLAG_AQ_REQUEST_STATS BIT(29)
>
> /* OS defined structs */
> struct net_device *netdev;
> @@ -399,6 +400,7 @@ int iavf_up(struct iavf_adapter *adapter);
> void iavf_down(struct iavf_adapter *adapter);
> int iavf_process_config(struct iavf_adapter *adapter);
> void iavf_schedule_reset(struct iavf_adapter *adapter);
> +void iavf_schedule_request_stats(struct iavf_adapter *adapter);
> void iavf_reset(struct iavf_adapter *adapter);
> void iavf_set_ethtool_ops(struct net_device *netdev);
> void iavf_update_stats(struct iavf_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 7cbe59beeebb..21c4d4180f3e 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -354,6 +354,9 @@ static void iavf_get_ethtool_stats(struct
> net_device *netdev,
> struct iavf_adapter *adapter = netdev_priv(netdev);
> unsigned int i;
>
> + /* Explicitly request stats refresh */
> + iavf_schedule_request_stats(adapter);
> +
> iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
>
> rcu_read_lock();
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 80437ef26391..e7ac6356772b 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -165,6 +165,19 @@ void iavf_schedule_reset(struct iavf_adapter
> *adapter)
> }
> }
>
> +/**
> + * iavf_schedule_request_stats - Set the flags and schedule
> statistics request
> + * @adapter: board private structure
> + *
> + * Sets IAVF_FLAG_AQ_REQUEST_STATS flag so iavf_watchdog_task() will
> explicitly
> + * request and refresh ethtool stats
> + **/
> +void iavf_schedule_request_stats(struct iavf_adapter *adapter)
> +{
> + adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_STATS;
This is never being cleared.
> + queue_work(iavf_wq, &adapter->watchdog_task.work);
> +}
> +
> /**
> * iavf_tx_timeout - Respond to a Tx Hang
> * @netdev: network interface device structure
> @@ -1700,6 +1713,11 @@ static int iavf_process_aq_command(struct
> iavf_adapter *adapter)
> iavf_del_adv_rss_cfg(adapter);
> return 0;
> }
> + if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_STATS) {
> + iavf_request_stats(adapter);
> + return IAVF_SUCCESS;
This function returns int, not iavf_status. This should be 0
> + }
>
> return -EAGAIN;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-09 20:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 6:48 [Intel-wired-lan] [PATCH net v1] iavf: Fix refreshing iavf adapter stats on ethtool request Jedrzej Jagielski
2021-09-03 7:43 ` Paul Menzel
2021-09-07 12:54 ` Jagielski, Jedrzej
2021-09-07 14:36 ` Paul Menzel
2021-09-09 20:38 ` Nguyen, Anthony L
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.