netdev - Re: [PATCH net-next v2] net: mvpp2: add ethtool GOP statistics

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, 6 Nov 2017 11:06:53 +0100
From:   Miquel RAYNAL <miquel.raynal@...e-electrons.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Antoine Tenart <antoine.tenart@...e-electrons.com>,
        Gregory Clement <gregory.clement@...e-electrons.com>,
        Nadav Haklai <nadavh@...vell.com>, netdev@...r.kernel.org,
        Stefan Chulski <stefanc@...vell.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next v2] net: mvpp2: add ethtool GOP statistics

Hi Andrew,

On Fri, 3 Nov 2017 16:19:06 +0100
Andrew Lunn <andrew@...n.ch> wrote:

> > @@ -817,6 +856,12 @@ struct mvpp2 {
> >  
> >  	/* Maximum number of RXQs per port */
> >  	unsigned int max_port_rxqs;
> > +
> > +	/* Workqueue to gather hardware statistics with its lock */
> > +	struct mutex gather_stats_lock;
> > +	struct delayed_work stats_work;
> > +	char queue_name[20];
> > +	struct workqueue_struct *stats_queue;
> >  };  
>   
> > +static u64 mvpp2_read_count(struct mvpp2_port *port,
> > +			    const struct mvpp2_ethtool_counter
> > *counter) +{
> > +	void __iomem *base;
> > +	u64 val;
> > +
> > +	if (port->priv->hw_version == MVPP21)
> > +		base = port->priv->lms_base +
> > MVPP21_MIB_COUNTERS_OFFSET +
> > +		       port->gop_id * MVPP21_MIB_COUNTERS_PORT_SZ;
> > +	else
> > +		base = port->priv->iface_base +
> > MVPP22_MIB_COUNTERS_OFFSET +
> > +		       port->gop_id *
> > MVPP22_MIB_COUNTERS_PORT_SZ;  
> 
> This seems like something which could be calculated once and then
> stored away, e.g. next to stats_queue.

Sure, it is even more important now that there is a workqueue calling
quite often this function. I actually had to move the new field into
the "mvpp2_port" structure otherwise the offset still should have been
calculated each time.

> 
> > +static void mvpp2_ethtool_get_stats(struct net_device *dev,
> > +				    struct ethtool_stats *stats,
> > u64 *data) +{
> > +	struct mvpp2_port *port = netdev_priv(dev);
> > +
> > +	/* Update statistics for all ports, copy only those
> > actually needed */
> > +	mvpp2_gather_hw_statistics(&port->priv->stats_work.work);
> > +
> > +	memcpy(data, port->ethtool_stats,
> > +	       sizeof(u64) * ARRAY_SIZE(mvpp2_ethtool_regs));  
> 
> Thanks for adding the mutex in mvpp2_gather_hw_statistics(). However,
> should we be holding the mutex while performing this copy? There is no
> snapshot support, so the statistics are not guaranteed to be
> consistent. However, since a statistics is a u64, it is possible the
> copy will get the new lower 32 bits and the old 32 bits if the copy
> happens while mvpp2_gather_hw_statistics() is running.

I thought the concurrency problem was only when reading from the
hardware registers so I did not thought about the situation you
describe here, I updated (see the coming v3) by surrounding the memcpy
call by acquiring/releasing the lock.

> 
> > +	port->ethtool_stats = devm_kcalloc(&pdev->dev,
> > +
> > ARRAY_SIZE(mvpp2_ethtool_regs),
> > +					   sizeof(u64),
> > GFP_KERNEL);
> > +	if (!port->ethtool_stats) {
> > +		err = -ENOMEM;
> > +		goto err_free_stats;
> > +	}
> > +
> >  	mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
> >  
> >  	port->tx_ring_size = MVPP2_MAX_TXD;
> > @@ -7707,6 +7904,7 @@ static void mvpp2_port_remove(struct
> > mvpp2_port *port) of_node_put(port->phy_node);
> >  	free_percpu(port->pcpu);
> >  	free_percpu(port->stats);
> > +	kfree(port->ethtool_stats);  
> 
> You allocate the memory using devm_. You should not use plain kfree()
> on it. You might want to spend some time reading about devm_

Rha, right, was too hurry to resend, I obviously forgot to update the
*_remove() function, sorry about that. 

> 
> > +	mutex_init(&priv->gather_stats_lock);
> > +	index = ida_simple_get(&engine_index_ida, 0, 0,
> > GFP_KERNEL);
> > +	if (index < 0)
> > +		goto err_mg_clk;
> > +
> > +	snprintf(priv->queue_name, sizeof(priv->queue_name),
> > +		 "mvpp2_stats_%d", index);  
> 
> I know Florian asked for unique names, which IDA will give you. But
> could you derive the name from device tree? It then becomes a name you
> can actually map back to the hardware, rather than being semi-random.

Here I do have a problem: I choose the IDA solution because it was
quite straightforward but I agree it would be better to use an unique
name. Unfortunately, on the Armada-8040-DB that instantiate this driver
twice, the node name is not unique. There are two CP (master and
slave) and both names are "ethernet@0". Otherwise, if I use the full
path, I get something like
"/cp110-master/config-space@...00000/ethernet@0" and
"/cp110-slave/config-space@...00000/ethernet@0" but the problem is that
workqueue names are truncated to 24 characters and only 15 appears in
ps, so it would not solve the issue and choosing the "parent
parent node name" would work here but does not scale very well. Do you
have any idea to get this right?

Thanks for the review,
Miquèl