Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a workqueue that grabs the information from time
to time from the hardware, and place it into the device structure,
so that, .ndo_get_stats quickly returns the device structure statistics
block.
Signed-off-by: Breno Leitao <redacted>
---
drivers/net/ehea/ehea.h | 1 +
drivers/net/ehea/ehea_main.c | 20 +++++++++++++++++---
2 files changed, 18 insertions(+), 3 deletions(-)
From: Eric Dumazet <hidden> Date: 2011-09-22 15:14:47
Le jeudi 22 septembre 2011 à 11:10 -0300, brenohl@br.ibm.com a écrit :
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a workqueue that grabs the information from time
to time from the hardware, and place it into the device structure,
so that, .ndo_get_stats quickly returns the device structure statistics
block.
Signed-off-by: Breno Leitao <redacted>
---
Hmm... trigerring a work queue everytime NAPI is run ? That seems very
expensive to me, if for example no one wants device stats at all.
I read ehea correctly, this blocking suff is only used to compute
stats->multicast and stats->rx_errors.
They probably could be updated once per second, nobody will complain.
(So use a workqueue, and trigger the job once per second, not once per
incoming packet)
Also, you dont provide a safe ndo_get_stats() implementation.
Think that several process might read your device stats in //
So the memset(stats, 0, sizeof(*stats)) is wrong :
You can clear stats that are currently read by another thread.
This makes SNMP readers read intermediate null values, and they dont
like this.
Fix is real easy : remove the memset(stats, 0, sizeof(*stats)) :
You only write on some fields, other fields are already 0
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 26 ++++++++++++++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
@@ -330,17 +330,24 @@ out:staticstructnet_device_stats*ehea_get_stats(structnet_device*dev){structehea_port*port=netdev_priv(dev);++return&port->stats;+}++staticvoidehea_update_stats(structwork_struct*work)+{+structehea_port*port=+container_of(work,structehea_port,stats_work.work);+structnet_device*dev=port->netdev;structnet_device_stats*stats=&port->stats;structhcp_ehea_port_cb2*cb2;u64hret,rx_packets,tx_packets,rx_bytes=0,tx_bytes=0;inti;-memset(stats,0,sizeof(*stats));-cb2=(void*)get_zeroed_page(GFP_KERNEL);if(!cb2){-netdev_err(dev,"no mem for cb2\n");-gotoout;+netdev_err(dev,"No mem for cb2. The interface statistics was not be updated\n");+gotoresched;}hret=ehea_h_query_ehea_port(port->adapter->handle,
From: Eric Dumazet <hidden> Date: 2011-09-26 18:02:16
Le lundi 26 septembre 2011 à 14:15 -0300, brenohl@br.ibm.com a écrit :
quoted hunk
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 26 ++++++++++++++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
Hmm...
You should move in ehea_get_stats() all software-computed stats.
Only the hardware assisted stats should be gathered/changed from your
ehea_update_stats() helper.
This way, SNMP readers get accurate stats for all fields but the
stats->multicast and stats->rx_errors that are updated once per second.
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 53 ++++++++++++++++------------
2 files changed, 31 insertions(+), 23 deletions(-)
@@ -331,16 +331,34 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev){structehea_port*port=netdev_priv(dev);structnet_device_stats*stats=&port->stats;-structhcp_ehea_port_cb2*cb2;-u64hret,rx_packets,tx_packets,rx_bytes=0,tx_bytes=0;inti;-memset(stats,0,sizeof(*stats));+for(i=0;i<port->num_def_qps;i++){+stats->rx_packets+=port->port_res[i].rx_packets;+stats->rx_bytes+=port->port_res[i].rx_bytes;+}++for(i=0;i<port->num_def_qps+port->num_add_tx_qps;i++){+stats->tx_packets+=port->port_res[i].tx_packets;+stats->tx_bytes+=port->port_res[i].tx_bytes;+}++return&port->stats;+}++staticvoidehea_update_stats(structwork_struct*work)+{+structehea_port*port=+container_of(work,structehea_port,stats_work.work);+structnet_device*dev=port->netdev;+structnet_device_stats*stats=&port->stats;+structhcp_ehea_port_cb2*cb2;+u64hret;cb2=(void*)get_zeroed_page(GFP_KERNEL);if(!cb2){-netdev_err(dev,"no mem for cb2\n");-gotoout;+netdev_err(dev,"No mem for cb2. Some interface statistics were not updated\n");+gotoresched;}hret=ehea_h_query_ehea_port(port->adapter->handle,
From: Eric Dumazet <hidden> Date: 2011-09-26 19:43:06
Le lundi 26 septembre 2011 à 16:21 -0300, brenohl@br.ibm.com a écrit :
quoted hunk
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 53 ++++++++++++++++------------
2 files changed, 31 insertions(+), 23 deletions(-)
No.
You _really_ need the temporary variables, and set stats->field once
final result is known. Right now, you are adding data over and over.
Please test your patches :(
quoted hunk
+ for (i = 0; i < port->num_def_qps + port->num_add_tx_qps; i++) {+ stats->tx_packets += port->port_res[i].tx_packets;+ stats->tx_bytes += port->port_res[i].tx_bytes;+ }++ return &port->stats;
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 59 +++++++++++++++++-----------
2 files changed, 37 insertions(+), 23 deletions(-)
@@ -331,16 +331,40 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev){structehea_port*port=netdev_priv(dev);structnet_device_stats*stats=&port->stats;-structhcp_ehea_port_cb2*cb2;-u64hret,rx_packets,tx_packets,rx_bytes=0,tx_bytes=0;+u64rx_packets=0,tx_packets=0,rx_bytes=0,tx_bytes=0;inti;-memset(stats,0,sizeof(*stats));+for(i=0;i<port->num_def_qps;i++){+rx_packets+=port->port_res[i].rx_packets;+rx_bytes+=port->port_res[i].rx_bytes;+}++for(i=0;i<port->num_def_qps+port->num_add_tx_qps;i++){+tx_packets+=port->port_res[i].tx_packets;+tx_bytes+=port->port_res[i].tx_bytes;+}++stats->tx_packets=tx_packets;+stats->rx_bytes=rx_bytes;+stats->tx_bytes=tx_bytes;+stats->rx_packets=rx_packets;++return&port->stats;+}++staticvoidehea_update_stats(structwork_struct*work)+{+structehea_port*port=+container_of(work,structehea_port,stats_work.work);+structnet_device*dev=port->netdev;+structnet_device_stats*stats=&port->stats;+structhcp_ehea_port_cb2*cb2;+u64hret;cb2=(void*)get_zeroed_page(GFP_KERNEL);if(!cb2){-netdev_err(dev,"no mem for cb2\n");-gotoout;+netdev_err(dev,"No mem for cb2. Some interface statistics were not updated\n");+gotoresched;}hret=ehea_h_query_ehea_port(port->adapter->handle,
From: Eric Dumazet <hidden> Date: 2011-09-26 20:29:02
Le lundi 26 septembre 2011 à 17:11 -0300, brenohl@br.ibm.com a écrit :
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
Seems good, thanks
Acked-by: Eric Dumazet <redacted>
Le lundi 26 septembre 2011 à 17:11 -0300, brenohl@br.ibm.com a écrit :
quoted
Currently ehea ndo_get_stats can sleep in two places, in a hcall
and in a GFP_KERNEL alloc, which is not correct.
This patch creates a delayed workqueue that grabs the information each 1
sec from the hardware, and place it into the device structure, so that,
.ndo_get_stats quickly returns the device structure statistics block.
Signed-off-by: Breno Leitao <redacted>
---
Seems good, thanks
Acked-by: Eric Dumazet <redacted>