* [PATCH net-next] ehea: Remove sleep at .ndo_get_stats
@ 2011-09-22 14:10 brenohl
2011-09-22 15:14 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: brenohl @ 2011-09-22 14:10 UTC (permalink / raw)
To: davem; +Cc: netdev, Breno Leitao
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 <leitao@linux.vnet.ibm.com>
---
drivers/net/ehea/ehea.h | 1 +
drivers/net/ehea/ehea_main.c | 20 +++++++++++++++++---
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ehea/ehea.h b/drivers/net/ehea/ehea.h
index 7dd5e6a..05c39ae 100644
--- a/drivers/net/ehea/ehea.h
+++ b/drivers/net/ehea/ehea.h
@@ -459,6 +459,7 @@ struct ehea_port {
struct ehea_mc_list *mc_list; /* Multicast MAC addresses */
struct ehea_eq *qp_eq;
struct work_struct reset_task;
+ struct work_struct stats_task;
struct mutex port_lock;
char int_aff_name[EHEA_IRQ_NAME_SIZE];
int allmulti; /* Indicates IFF_ALLMULTI state */
diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
index be2cb4a..7465553 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -330,6 +330,15 @@ out:
static struct net_device_stats *ehea_get_stats(struct net_device *dev)
{
struct ehea_port *port = netdev_priv(dev);
+
+ return &port->stats;
+}
+
+static void ehea_update_stats(struct work_struct *work)
+{
+ struct ehea_port *port =
+ container_of(work, struct ehea_port, stats_task);
+ struct net_device *dev = port->netdev;
struct net_device_stats *stats = &port->stats;
struct hcp_ehea_port_cb2 *cb2;
u64 hret, rx_packets, tx_packets, rx_bytes = 0, tx_bytes = 0;
@@ -340,7 +349,7 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
cb2 = (void *)get_zeroed_page(GFP_KERNEL);
if (!cb2) {
netdev_err(dev, "no mem for cb2\n");
- goto out;
+ return;
}
hret = ehea_h_query_ehea_port(port->adapter->handle,
@@ -375,8 +384,6 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
out_herr:
free_page((unsigned long)cb2);
-out:
- return stats;
}
static void ehea_refill_rq1(struct ehea_port_res *pr, int index, int nr_of_wqes)
@@ -789,6 +796,8 @@ static int ehea_proc_rwqes(struct net_device *dev,
ehea_refill_rq2(pr, processed_rq2);
ehea_refill_rq3(pr, processed_rq3);
+ schedule_work(&port->stats_task);
+
return processed;
}
@@ -953,6 +962,7 @@ static int ehea_poll(struct napi_struct *napi, int budget)
rx += ehea_proc_rwqes(dev, pr, budget - rx);
}
+ ehea_update_stats(dev);
pr->poll_counter++;
return rx;
}
@@ -2651,6 +2661,7 @@ static int ehea_open(struct net_device *dev)
}
mutex_unlock(&port->port_lock);
+ schedule_work(&port->stats_task);
return ret;
}
@@ -2690,6 +2701,7 @@ static int ehea_stop(struct net_device *dev)
set_bit(__EHEA_DISABLE_PORT_RESET, &port->flags);
cancel_work_sync(&port->reset_task);
+ cancel_work_sync(&port->stats_task);
mutex_lock(&port->port_lock);
netif_stop_queue(dev);
port_napi_disable(port);
@@ -3235,6 +3247,7 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
dev->features |= NETIF_F_LRO;
INIT_WORK(&port->reset_task, ehea_reset_port);
+ INIT_WORK(&port->stats_task, ehea_update_stats);
init_waitqueue_head(&port->swqe_avail_wq);
init_waitqueue_head(&port->restart_wq);
@@ -3278,6 +3291,7 @@ static void ehea_shutdown_single_port(struct ehea_port *port)
struct ehea_adapter *adapter = port->adapter;
cancel_work_sync(&port->reset_task);
+ cancel_work_sync(&port->stats_task);
unregister_netdev(port->netdev);
ehea_unregister_port(port);
kfree(port->mc_list);
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] ehea: Remove sleep at .ndo_get_stats
2011-09-22 14:10 [PATCH net-next] ehea: Remove sleep at .ndo_get_stats brenohl
@ 2011-09-22 15:14 ` Eric Dumazet
2011-09-26 17:15 ` [PATCH] " brenohl
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-09-22 15:14 UTC (permalink / raw)
To: brenohl; +Cc: davem, netdev, Breno Leitao
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 <leitao@linux.vnet.ibm.com>
> ---
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-22 15:14 ` Eric Dumazet
@ 2011-09-26 17:15 ` brenohl
2011-09-26 18:02 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: brenohl @ 2011-09-26 17:15 UTC (permalink / raw)
To: eric.dumazet; +Cc: davem, netdev, Breno Leitao
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 <brenohl@br.ibm.com>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 26 ++++++++++++++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ehea/ehea.h b/drivers/net/ethernet/ibm/ehea/ehea.h
index 7dd5e6a..0b8e6a9 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea.h
+++ b/drivers/net/ethernet/ibm/ehea/ehea.h
@@ -459,6 +459,7 @@ struct ehea_port {
struct ehea_mc_list *mc_list; /* Multicast MAC addresses */
struct ehea_eq *qp_eq;
struct work_struct reset_task;
+ struct delayed_work stats_work;
struct mutex port_lock;
char int_aff_name[EHEA_IRQ_NAME_SIZE];
int allmulti; /* Indicates IFF_ALLMULTI state */
diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
index 583bcd3..a10f6b3 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
+++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
@@ -330,17 +330,24 @@ out:
static struct net_device_stats *ehea_get_stats(struct net_device *dev)
{
struct ehea_port *port = netdev_priv(dev);
+
+ return &port->stats;
+}
+
+static void ehea_update_stats(struct work_struct *work)
+{
+ struct ehea_port *port =
+ container_of(work, struct ehea_port, stats_work.work);
+ struct net_device *dev = port->netdev;
struct net_device_stats *stats = &port->stats;
struct hcp_ehea_port_cb2 *cb2;
u64 hret, rx_packets, tx_packets, rx_bytes = 0, tx_bytes = 0;
int i;
- memset(stats, 0, sizeof(*stats));
-
cb2 = (void *)get_zeroed_page(GFP_KERNEL);
if (!cb2) {
- netdev_err(dev, "no mem for cb2\n");
- goto out;
+ netdev_err(dev, "No mem for cb2. The interface statistics was not be updated\n");
+ goto resched;
}
hret = ehea_h_query_ehea_port(port->adapter->handle,
@@ -375,8 +382,10 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
out_herr:
free_page((unsigned long)cb2);
-out:
- return stats;
+
+resched:
+ schedule_delayed_work(&port->stats_work, msecs_to_jiffies(1000));
+
}
static void ehea_refill_rq1(struct ehea_port_res *pr, int index, int nr_of_wqes)
@@ -2651,6 +2660,7 @@ static int ehea_open(struct net_device *dev)
}
mutex_unlock(&port->port_lock);
+ schedule_delayed_work(&port->stats_work, msecs_to_jiffies(1000));
return ret;
}
@@ -2690,6 +2700,7 @@ static int ehea_stop(struct net_device *dev)
set_bit(__EHEA_DISABLE_PORT_RESET, &port->flags);
cancel_work_sync(&port->reset_task);
+ cancel_delayed_work_sync(&port->stats_work);
mutex_lock(&port->port_lock);
netif_stop_queue(dev);
port_napi_disable(port);
@@ -3235,10 +3246,12 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
dev->features |= NETIF_F_LRO;
INIT_WORK(&port->reset_task, ehea_reset_port);
+ INIT_DELAYED_WORK(&port->stats_work, ehea_update_stats);
init_waitqueue_head(&port->swqe_avail_wq);
init_waitqueue_head(&port->restart_wq);
+ memset(&port->stats, 0, sizeof(struct net_device_stats));
ret = register_netdev(dev);
if (ret) {
pr_err("register_netdev failed. ret=%d\n", ret);
@@ -3278,6 +3291,7 @@ static void ehea_shutdown_single_port(struct ehea_port *port)
struct ehea_adapter *adapter = port->adapter;
cancel_work_sync(&port->reset_task);
+ cancel_delayed_work_sync(&port->stats_work);
unregister_netdev(port->netdev);
ehea_unregister_port(port);
kfree(port->mc_list);
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 17:15 ` [PATCH] " brenohl
@ 2011-09-26 18:02 ` Eric Dumazet
2011-09-26 18:56 ` Breno Leitao
2011-09-26 19:21 ` brenohl
0 siblings, 2 replies; 10+ messages in thread
From: Eric Dumazet @ 2011-09-26 18:02 UTC (permalink / raw)
To: brenohl; +Cc: davem, netdev
Le lundi 26 septembre 2011 à 14:15 -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 <brenohl@br.ibm.com>
> ---
> drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
> drivers/net/ethernet/ibm/ehea/ehea_main.c | 26 ++++++++++++++++++++------
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ehea/ehea.h b/drivers/net/ethernet/ibm/ehea/ehea.h
> index 7dd5e6a..0b8e6a9 100644
> --- a/drivers/net/ethernet/ibm/ehea/ehea.h
> +++ b/drivers/net/ethernet/ibm/ehea/ehea.h
> @@ -459,6 +459,7 @@ struct ehea_port {
> struct ehea_mc_list *mc_list; /* Multicast MAC addresses */
> struct ehea_eq *qp_eq;
> struct work_struct reset_task;
> + struct delayed_work stats_work;
> struct mutex port_lock;
> char int_aff_name[EHEA_IRQ_NAME_SIZE];
> int allmulti; /* Indicates IFF_ALLMULTI state */
> diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
> index 583bcd3..a10f6b3 100644
> --- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
> +++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
> @@ -330,17 +330,24 @@ out:
> static struct net_device_stats *ehea_get_stats(struct net_device *dev)
> {
> struct ehea_port *port = netdev_priv(dev);
> +
> + return &port->stats;
> +}
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 18:02 ` Eric Dumazet
@ 2011-09-26 18:56 ` Breno Leitao
2011-09-26 19:21 ` brenohl
1 sibling, 0 replies; 10+ messages in thread
From: Breno Leitao @ 2011-09-26 18:56 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev
On 09/26/2011 03:02 PM, Eric Dumazet wrote:
> You should move in ehea_get_stats() all software-computed stats.
Agreed. I am going to resend the patch.
Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 18:02 ` Eric Dumazet
2011-09-26 18:56 ` Breno Leitao
@ 2011-09-26 19:21 ` brenohl
2011-09-26 19:42 ` Eric Dumazet
1 sibling, 1 reply; 10+ messages in thread
From: brenohl @ 2011-09-26 19:21 UTC (permalink / raw)
To: eric.dumazet; +Cc: davem, netdev, Breno Leitao
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 <brenohl@br.ibm.com>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 53 ++++++++++++++++------------
2 files changed, 31 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ehea/ehea.h b/drivers/net/ethernet/ibm/ehea/ehea.h
index 7dd5e6a..0b8e6a9 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea.h
+++ b/drivers/net/ethernet/ibm/ehea/ehea.h
@@ -459,6 +459,7 @@ struct ehea_port {
struct ehea_mc_list *mc_list; /* Multicast MAC addresses */
struct ehea_eq *qp_eq;
struct work_struct reset_task;
+ struct delayed_work stats_work;
struct mutex port_lock;
char int_aff_name[EHEA_IRQ_NAME_SIZE];
int allmulti; /* Indicates IFF_ALLMULTI state */
diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
index 583bcd3..976988d 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
+++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
@@ -331,16 +331,34 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
{
struct ehea_port *port = netdev_priv(dev);
struct net_device_stats *stats = &port->stats;
- struct hcp_ehea_port_cb2 *cb2;
- u64 hret, rx_packets, tx_packets, rx_bytes = 0, tx_bytes = 0;
int i;
- 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;
+}
+
+static void ehea_update_stats(struct work_struct *work)
+{
+ struct ehea_port *port =
+ container_of(work, struct ehea_port, stats_work.work);
+ struct net_device *dev = port->netdev;
+ struct net_device_stats *stats = &port->stats;
+ struct hcp_ehea_port_cb2 *cb2;
+ u64 hret;
cb2 = (void *)get_zeroed_page(GFP_KERNEL);
if (!cb2) {
- netdev_err(dev, "no mem for cb2\n");
- goto out;
+ netdev_err(dev, "No mem for cb2. Some interface statistics were not updated\n");
+ goto resched;
}
hret = ehea_h_query_ehea_port(port->adapter->handle,
@@ -354,29 +372,13 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
if (netif_msg_hw(port))
ehea_dump(cb2, sizeof(*cb2), "net_device_stats");
- rx_packets = 0;
- 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;
- }
-
- tx_packets = 0;
- 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->multicast = cb2->rxmcp;
stats->rx_errors = cb2->rxuerr;
- stats->rx_bytes = rx_bytes;
- stats->tx_bytes = tx_bytes;
- stats->rx_packets = rx_packets;
out_herr:
free_page((unsigned long)cb2);
-out:
- return stats;
+resched:
+ schedule_delayed_work(&port->stats_work, msecs_to_jiffies(1000));
}
static void ehea_refill_rq1(struct ehea_port_res *pr, int index, int nr_of_wqes)
@@ -2651,6 +2653,7 @@ static int ehea_open(struct net_device *dev)
}
mutex_unlock(&port->port_lock);
+ schedule_delayed_work(&port->stats_work, msecs_to_jiffies(1000));
return ret;
}
@@ -2690,6 +2693,7 @@ static int ehea_stop(struct net_device *dev)
set_bit(__EHEA_DISABLE_PORT_RESET, &port->flags);
cancel_work_sync(&port->reset_task);
+ cancel_delayed_work_sync(&port->stats_work);
mutex_lock(&port->port_lock);
netif_stop_queue(dev);
port_napi_disable(port);
@@ -3235,10 +3239,12 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
dev->features |= NETIF_F_LRO;
INIT_WORK(&port->reset_task, ehea_reset_port);
+ INIT_DELAYED_WORK(&port->stats_work, ehea_update_stats);
init_waitqueue_head(&port->swqe_avail_wq);
init_waitqueue_head(&port->restart_wq);
+ memset(&port->stats, 0, sizeof(struct net_device_stats));
ret = register_netdev(dev);
if (ret) {
pr_err("register_netdev failed. ret=%d\n", ret);
@@ -3278,6 +3284,7 @@ static void ehea_shutdown_single_port(struct ehea_port *port)
struct ehea_adapter *adapter = port->adapter;
cancel_work_sync(&port->reset_task);
+ cancel_delayed_work_sync(&port->stats_work);
unregister_netdev(port->netdev);
ehea_unregister_port(port);
kfree(port->mc_list);
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 19:21 ` brenohl
@ 2011-09-26 19:42 ` Eric Dumazet
2011-09-26 20:11 ` brenohl
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-09-26 19:42 UTC (permalink / raw)
To: brenohl; +Cc: davem, netdev
Le lundi 26 septembre 2011 à 16:21 -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 <brenohl@br.ibm.com>
> ---
> drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
> drivers/net/ethernet/ibm/ehea/ehea_main.c | 53 ++++++++++++++++------------
> 2 files changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ehea/ehea.h b/drivers/net/ethernet/ibm/ehea/ehea.h
> index 7dd5e6a..0b8e6a9 100644
> --- a/drivers/net/ethernet/ibm/ehea/ehea.h
> +++ b/drivers/net/ethernet/ibm/ehea/ehea.h
> @@ -459,6 +459,7 @@ struct ehea_port {
> struct ehea_mc_list *mc_list; /* Multicast MAC addresses */
> struct ehea_eq *qp_eq;
> struct work_struct reset_task;
> + struct delayed_work stats_work;
> struct mutex port_lock;
> char int_aff_name[EHEA_IRQ_NAME_SIZE];
> int allmulti; /* Indicates IFF_ALLMULTI state */
> diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
> index 583bcd3..976988d 100644
> --- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
> +++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
> @@ -331,16 +331,34 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
> {
> struct ehea_port *port = netdev_priv(dev);
> struct net_device_stats *stats = &port->stats;
> - struct hcp_ehea_port_cb2 *cb2;
> - u64 hret, rx_packets, tx_packets, rx_bytes = 0, tx_bytes = 0;
> int i;
>
> - 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;
> + }
> +
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 :(
> + 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;
return stats;
> +}
> +
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 19:42 ` Eric Dumazet
@ 2011-09-26 20:11 ` brenohl
2011-09-26 20:28 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: brenohl @ 2011-09-26 20:11 UTC (permalink / raw)
To: eric.dumazet; +Cc: davem, netdev, Breno Leitao
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 <brenohl@br.ibm.com>
---
drivers/net/ethernet/ibm/ehea/ehea.h | 1 +
drivers/net/ethernet/ibm/ehea/ehea_main.c | 59 +++++++++++++++++-----------
2 files changed, 37 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ehea/ehea.h b/drivers/net/ethernet/ibm/ehea/ehea.h
index 7dd5e6a..0b8e6a9 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea.h
+++ b/drivers/net/ethernet/ibm/ehea/ehea.h
@@ -459,6 +459,7 @@ struct ehea_port {
struct ehea_mc_list *mc_list; /* Multicast MAC addresses */
struct ehea_eq *qp_eq;
struct work_struct reset_task;
+ struct delayed_work stats_work;
struct mutex port_lock;
char int_aff_name[EHEA_IRQ_NAME_SIZE];
int allmulti; /* Indicates IFF_ALLMULTI state */
diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
index 583bcd3..c821cb6 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
+++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
@@ -331,16 +331,40 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
{
struct ehea_port *port = netdev_priv(dev);
struct net_device_stats *stats = &port->stats;
- struct hcp_ehea_port_cb2 *cb2;
- u64 hret, rx_packets, tx_packets, rx_bytes = 0, tx_bytes = 0;
+ u64 rx_packets = 0, tx_packets = 0, rx_bytes = 0, tx_bytes = 0;
int i;
- 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;
+}
+
+static void ehea_update_stats(struct work_struct *work)
+{
+ struct ehea_port *port =
+ container_of(work, struct ehea_port, stats_work.work);
+ struct net_device *dev = port->netdev;
+ struct net_device_stats *stats = &port->stats;
+ struct hcp_ehea_port_cb2 *cb2;
+ u64 hret;
cb2 = (void *)get_zeroed_page(GFP_KERNEL);
if (!cb2) {
- netdev_err(dev, "no mem for cb2\n");
- goto out;
+ netdev_err(dev, "No mem for cb2. Some interface statistics were not updated\n");
+ goto resched;
}
hret = ehea_h_query_ehea_port(port->adapter->handle,
@@ -354,29 +378,13 @@ static struct net_device_stats *ehea_get_stats(struct net_device *dev)
if (netif_msg_hw(port))
ehea_dump(cb2, sizeof(*cb2), "net_device_stats");
- rx_packets = 0;
- 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;
- }
-
- tx_packets = 0;
- 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->multicast = cb2->rxmcp;
stats->rx_errors = cb2->rxuerr;
- stats->rx_bytes = rx_bytes;
- stats->tx_bytes = tx_bytes;
- stats->rx_packets = rx_packets;
out_herr:
free_page((unsigned long)cb2);
-out:
- return stats;
+resched:
+ schedule_delayed_work(&port->stats_work, msecs_to_jiffies(1000));
}
static void ehea_refill_rq1(struct ehea_port_res *pr, int index, int nr_of_wqes)
@@ -2651,6 +2659,7 @@ static int ehea_open(struct net_device *dev)
}
mutex_unlock(&port->port_lock);
+ schedule_delayed_work(&port->stats_work, msecs_to_jiffies(1000));
return ret;
}
@@ -2690,6 +2699,7 @@ static int ehea_stop(struct net_device *dev)
set_bit(__EHEA_DISABLE_PORT_RESET, &port->flags);
cancel_work_sync(&port->reset_task);
+ cancel_delayed_work_sync(&port->stats_work);
mutex_lock(&port->port_lock);
netif_stop_queue(dev);
port_napi_disable(port);
@@ -3235,10 +3245,12 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
dev->features |= NETIF_F_LRO;
INIT_WORK(&port->reset_task, ehea_reset_port);
+ INIT_DELAYED_WORK(&port->stats_work, ehea_update_stats);
init_waitqueue_head(&port->swqe_avail_wq);
init_waitqueue_head(&port->restart_wq);
+ memset(&port->stats, 0, sizeof(struct net_device_stats));
ret = register_netdev(dev);
if (ret) {
pr_err("register_netdev failed. ret=%d\n", ret);
@@ -3278,6 +3290,7 @@ static void ehea_shutdown_single_port(struct ehea_port *port)
struct ehea_adapter *adapter = port->adapter;
cancel_work_sync(&port->reset_task);
+ cancel_delayed_work_sync(&port->stats_work);
unregister_netdev(port->netdev);
ehea_unregister_port(port);
kfree(port->mc_list);
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 20:11 ` brenohl
@ 2011-09-26 20:28 ` Eric Dumazet
2011-09-27 4:47 ` David Miller
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-09-26 20:28 UTC (permalink / raw)
To: brenohl; +Cc: davem, netdev
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 <brenohl@br.ibm.com>
> ---
Seems good, thanks
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ehea: Remove sleep at .ndo_get_stats
2011-09-26 20:28 ` Eric Dumazet
@ 2011-09-27 4:47 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2011-09-27 4:47 UTC (permalink / raw)
To: eric.dumazet; +Cc: brenohl, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 26 Sep 2011 22:28:55 +0200
> 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 <brenohl@br.ibm.com>
>> ---
>
> Seems good, thanks
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-09-27 4:47 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-22 14:10 [PATCH net-next] ehea: Remove sleep at .ndo_get_stats brenohl
2011-09-22 15:14 ` Eric Dumazet
2011-09-26 17:15 ` [PATCH] " brenohl
2011-09-26 18:02 ` Eric Dumazet
2011-09-26 18:56 ` Breno Leitao
2011-09-26 19:21 ` brenohl
2011-09-26 19:42 ` Eric Dumazet
2011-09-26 20:11 ` brenohl
2011-09-26 20:28 ` Eric Dumazet
2011-09-27 4:47 ` David Miller
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.