netdev - Re: [PATCH] ehea: Remove sleep at .ndo_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 26 Sep 2011 21:42:59 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	brenohl@...ibm.com
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] ehea: Remove sleep at .ndo_get_stats

Le lundi 26 septembre 2011 à 16:21 -0300, brenohl@...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@...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;

> +}
> +


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html