netdev - Re: [PATCH] net: add support for STMicroelectronics Ethernet controllers.

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]

Message-ID: <4ACB3A42.9070600@st.com>
Date:	Tue, 06 Oct 2009 14:38:26 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] net: add support for STMicroelectronics Ethernet controllers.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Eric,
many thanks for your prompt feedback.

Eric Dumazet wrote:
> Giuseppe CAVALLARO a écrit :
> 
>> +static int stmmac_sw_tso(struct stmmac_priv *priv, struct sk_buff *skb)
[snip]
> 
> So stmmac_sw_tso() calls stmmac_xmit() for each seg skb.
> 
> But stmmac_sw_tso() was called from stmmac_xmit(),
> with priv->tx_lock locked, so I suspect something is wrong.

Yes, you are right on this.
I'm going to remove it. Indeed, I observed no gain during my performance
tests. I had added it just to become familiar with this.
Note also that our chips do not have the segmentation offloading in Hw.
Thanks for this observation.

> Also please change various
> 
> +	mac = kmalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
> +	memset(mac, 0, sizeof(struct mac_device_info));
> +
> 
> +	mac = kmalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
> +	memset(mac, 0, sizeof(struct mac_device_info));
> +
> 
> to use kzalloc(), and of course, you should check kmalloc()/kzalloc() dont return NULL !
> 

Yes I'll do that too.

> Also :
> 
> +static int stmmac_clean_tx(struct net_device *dev)
[snip]
> +		if (skb != NULL) {
> +			/*
> +			 * If there's room in the queue (limit it to size)
> +			 * we add this skb back into the pool,
> +			 * if it's the right size.
> +			 */
> +			if ((skb_queue_len(&priv->rx_recycle) <
> +				priv->dma_rx_size) &&
> +				skb_recycle_check(skb, priv->dma_buf_sz))
> +				__skb_queue_head(&priv->rx_recycle, skb);
> +			else
> +				dev_kfree_skb_any(skb);
> 
> Why call dev_kfree_skb_any() here ? From NAPI context it is overkill.

The logic behind this piece of code should be the same one adopted in
other drivers like gianfar, ucc_geth and mv643xx_eth. What am I missing?

> static int stmmac_poll(struct napi_struct *napi, int budget)
> +{
[snip]
> +
> +	tx_cleaned = stmmac_clean_tx(dev);
> +
> +	work_done = stmmac_rx(dev, budget);
> +
> 
> 
> +	if (tx_cleaned)
> +		return budget;
> 
> Why tx_cleaned is used here to exit early ?

I've found interesting the approach used in gianfar (see commit
42199884594bc336c9185441cbed99a9324dab34).

Thanks again for the feedback.

Regards,
Peppe
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrLOjkACgkQ2Xo3j31MSSIM5ACgrTLJgwO84ooKkcoEaNMZ5bcy
iBUAoK+q677OnyeCdeNndFq92AmwNPoa
=r7WK
-----END PGP SIGNATURE-----
--
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