Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542598 > unrolled thread

Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue lock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue  lock Pavel Machek <pavel@ucw.cz> - 2016-12-15 10:50 +0100
    Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue  lock Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-15 11:10 +0100
      Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue  lock Pavel Machek <pavel@ucw.cz> - 2016-12-15 11:50 +0100
    Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue  lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-15 20:50 +0100
      Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue  lock Pavel Machek <pavel@ucw.cz> - 2016-12-15 23:10 +0100

#1542598 — Re: [PATCH v2 2/2] net: ethernet: stmmac: remove private tx queue lock

[Multipart message — attachments visible in raw view] — view raw

Hi!

> The driver uses a private lock for synchronization of the xmit function and
> the xmit completion handler, but since the NETIF_F_LLTX flag is not set,
> the xmit function is also called with the xmit_lock held.
> 
> On the other hand the completion handler uses the reverse locking order by
> first taking the private lock and (in case that the tx queue had been
> stopped) then the xmit_lock.
> 
> Improve the locking by removing the private lock and using only the
> xmit_lock for synchronization instead.

Do you have stmmac hardware to test on?

I believe something is very wrong with the locking there. In
particular... scheduling the stmmac_tx_timer() function to run often
should not do anything bad if locking is correct... but it breaks the
driver rather quickly. [Example patch below, needs applying to two
places in net-next.]

(Other possibility is that hardware races with the driver.)

Giuseppe, is there documentation available for the chip? Driver says

  Documentation available at:
          http://www.stlinux.com

but that page does not work for me...

404 Not Found

Code: NoSuchBucket
Message: The specified bucket does not exist
BucketName: www.stlinux.com
RequestId: 1C8A20CB99AE7F75
HostId:
ljPnqbEpyD8exct5MUgcDXSW8n+I67Yw0aejNhLuBQ0pqN0UCfiRBa3ztlOMngiXoSN+COX+VSw=

Best regards,
									Pavel

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ffbcd03..8040370 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1973,8 +1973,9 @@ static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int
 	 */
 	priv->tx_count_frames += nfrags + 1;
 	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
-		mod_timer(&priv->txtimer,
-			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
+		if (priv->tx_count_frames == nfrags + 1)
+			mod_timer(&priv->txtimer,
+				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
 	} else {
 		priv->tx_count_frames = 0;
 		priv->hw->desc->set_tx_ic(desc);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1542609

On 12/15/2016 10:45 AM, Pavel Machek wrote:
> Giuseppe, is there documentation available for the chip? Driver says
>
>   Documentation available at:
>           http://www.stlinux.com
>
> but that page does not work for me...

Hi Pavel, yes the page has been removed but all the relevant and
updated driver doc is inside the kernel sources.

Regards
Peppe

[toc] | [prev] | [next] | [standalone]

#1542628

[Multipart message — attachments visible in raw view] — view raw

On Thu 2016-12-15 11:08:36, Giuseppe CAVALLARO wrote:
> On 12/15/2016 10:45 AM, Pavel Machek wrote:
> >Giuseppe, is there documentation available for the chip? Driver says
> >
> >  Documentation available at:
> >          http://www.stlinux.com
> >
> >but that page does not work for me...
> 
> Hi Pavel, yes the page has been removed but all the relevant and
> updated driver doc is inside the kernel sources.

Ok, perhaps the link should be removed, then? (Along with the bugzilla
link if that is not going to be re-enabled?)

Is there documentation for the hardware somewhere?

(As something is very wrong with stmmac_tx_clean(), either locking or
interface to the DMA engine.)

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1543001

Hi,

On 15.12.2016 10:45, Pavel Machek wrote:
> Hi!
> 
>> The driver uses a private lock for synchronization of the xmit function and
>> the xmit completion handler, but since the NETIF_F_LLTX flag is not set,
>> the xmit function is also called with the xmit_lock held.
>> 
>> On the other hand the completion handler uses the reverse locking order by
>> first taking the private lock and (in case that the tx queue had been
>> stopped) then the xmit_lock.
>> 
>> Improve the locking by removing the private lock and using only the
>> xmit_lock for synchronization instead.
> 
> Do you have stmmac hardware to test on?
> 

Unfortunately not (I mentioned that the patch I send was only compile tested in 
the first version but I think I forgot to do so in the last version).

> I believe something is very wrong with the locking there. In
> particular... scheduling the stmmac_tx_timer() function to run often
> should not do anything bad if locking is correct... but it breaks the
> driver rather quickly. [Example patch below, needs applying to two
> places in net-next.]
> 

Do you get this result only after the private lock is removed? Or has this problem
been there before? And how exactly does the failure look like?

Regards,
Lino  

[toc] | [prev] | [next] | [standalone]

#1543086

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >> The driver uses a private lock for synchronization of the xmit function and
> >> the xmit completion handler, but since the NETIF_F_LLTX flag is not set,
> >> the xmit function is also called with the xmit_lock held.
> >> 
> >> On the other hand the completion handler uses the reverse locking order by
> >> first taking the private lock and (in case that the tx queue had been
> >> stopped) then the xmit_lock.
> >> 
> >> Improve the locking by removing the private lock and using only the
> >> xmit_lock for synchronization instead.
> > 
> > Do you have stmmac hardware to test on?
> > 
> 
> Unfortunately not (I mentioned that the patch I send was only compile tested in 
> the first version but I think I forgot to do so in the last
> version).

:-(.


> > I believe something is very wrong with the locking there. In
> > particular... scheduling the stmmac_tx_timer() function to run often
> > should not do anything bad if locking is correct... but it breaks the
> > driver rather quickly. [Example patch below, needs applying to two
> > places in net-next.]
> > 
> 
> Do you get this result only after the private lock is removed? Or has this problem
> been there before? And how exactly does the failure look like?

I believe I was getting very similar fun even with the private lock. I
re-applied the private lock, and the result is the same.

Also.. locking does seems to work. I added checks to see if the
stmmac_tx_clean() and stmmac_xmit() run at the same time, and they
don't seem to. So my best guess at the moment is missing cache flush
or mb() somewhere.

Failure looks like this:

root@wagabuibui:~# mount /dev/mmcblk0p4 /mnt
o 1000000 > /proc/sys/net/core/wmeroot@wagabuibui:~# chroot /mnt
/bin/bash
root@wagabuibui:/# mount /proc000 100 30
root@wagabuibui:/# #echo 1000000 > /proc/sys/net/core/wmem_default
root@wagabuibui:/# cd /data/tmp/udpt
root@wagabuibui:/data/tmp/udpt# ifconfig eth0 10.0.0.170 up
[   18.358072] socfpga-dwmac ff702000.ethernet eth0: IEEE 1588-2008
Advanced Timestamp supported
[   18.366836] socfpga-dwmac ff702000.ethernet eth0: registered PTP
clock
root@wagabuibui:/data/tmp/udpt# ./udp-test raw 10.0.0.6 1234 1000 100
30
Sending 100 packets (1000b each) at an interval of 30ms, expected data
rate:3333333b/s (3373333b/s incl udp overhead)
[   20.453538] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
100Mbps/Full - flow control rx/tx
[   20.581826] Link is Up - 100/Full
Sending UDP packet took >10ms: 5205162us
This would lead to a lost frame!
Sending UDP packet took >10ms: 40010us
This would lead to a lost frame!
Sending UDP packet took >10ms: 6366084us
This would lead to a lost frame!
Sending UDP packet took >10ms: 36971us
This would lead to a lost frame!
[   42.084940] ------------[ cut here ]------------
[   42.089577] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
dev_watchdog+0x254/0x26c
[   42.097821] NETDEV WATCHDOG: eth0 (socfpga-dwmac): transmit queue 0
timed out
[   42.104935] Modules linked in:

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web