Patchwork [1/1] via-rhine: Fix hanging with high CPU load on low-end broads.

login
register
mail settings
Submitter françois romieu
Date Dec. 30, 2011, 9:50 p.m.
Message ID <20111230215024.GA5513@electric-eye.fr.zoreil.com>
Download mbox | patch
Permalink /patch/133696/
State RFC
Delegated to: David Miller
Headers show

Comments

françois romieu - Dec. 30, 2011, 9:50 p.m.
Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
[...]
> Just applied it to HEAD of net-next, and got the following compiler warning:
> drivers/net/ethernet/via/via-rhine.c:2182:13: warning:
> 'rhine_task_enable' defined but not used

You are running with CONFIG_PM disabled and a down / up loop will not
work. The patch below should hide the latter, at least as long as the
relevant workqueue does not stall for too long... Extra cancel_work in
task_enable could work too but it will look strange.

> Just so you know that too :)

Thanks for testing.
Bjarke Istrup Pedersen - Dec. 30, 2011, 10:08 p.m.
2011/12/30 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Just applied it to HEAD of net-next, and got the following compiler warning:
>> drivers/net/ethernet/via/via-rhine.c:2182:13: warning:
>> 'rhine_task_enable' defined but not used
>
> You are running with CONFIG_PM disabled and a down / up loop will not
> work. The patch below should hide the latter, at least as long as the
> relevant workqueue does not stall for too long... Extra cancel_work in
> task_enable could work too but it will look strange.

Applied the patch, testing later :)
You are saying that with CONFIG_PM disabled, I wont be able to bring
the interface down and up again?

>> Just so you know that too :)
>
> Thanks for testing.
>
> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index c57e1da..89ced1b 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -1536,6 +1536,7 @@ static int rhine_open(struct net_device *dev)
>        alloc_rbufs(dev);
>        alloc_tbufs(dev);
>        rhine_chip_reset(dev);
> +       rhine_task_enable(rp);
>        init_registers(dev);
>        if (debug > 2)
>                netdev_dbg(dev, "%s() Done - status %04x MII status: %04x\n",
> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - Dec. 30, 2011, 10:08 p.m.
Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
[...]
> Applied the patch, testing later :)
> You are saying that with CONFIG_PM disabled, I wont be able to bring
> the interface down and up again?

No. You would not see the message with CONFIG_PM but the down / up problem
would still be there. The patch should fix the down / up problem.

/away
Bjarke Istrup Pedersen - Dec. 30, 2011, 10:13 p.m.
2011/12/30 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Just applied it to HEAD of net-next, and got the following compiler warning:
>> drivers/net/ethernet/via/via-rhine.c:2182:13: warning:
>> 'rhine_task_enable' defined but not used
>
> You are running with CONFIG_PM disabled and a down / up loop will not
> work. The patch below should hide the latter, at least as long as the
> relevant workqueue does not stall for too long... Extra cancel_work in
> task_enable could work too but it will look strange.
>
>> Just so you know that too :)

Does not build with patch:

drivers/net/ethernet/via/via-rhine.c: In function 'rhine_open':
drivers/net/ethernet/via/via-rhine.c:1539:2: error: implicit
declaration of function 'rhine_task_enable'
drivers/net/ethernet/via/via-rhine.c: At top level:
drivers/net/ethernet/via/via-rhine.c:2183:13: warning: conflicting
types for 'rhine_task_enable'
drivers/net/ethernet/via/via-rhine.c:2183:13: error: static
declaration of 'rhine_task_enable' follows non-static declaration
drivers/net/ethernet/via/via-rhine.c:1539:2: note: previous implicit
declaration of 'rhine_task_enable' was here
drivers/net/ethernet/via/via-rhine.c:2183:13: warning:
'rhine_task_enable' defined but not used

Wouldn't it be an idea to move the declarations to a via-rhine.h file,
and include it, to work around it, and seperate it like it should be?

/Bjarke

> Thanks for testing.
>
> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index c57e1da..89ced1b 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -1536,6 +1536,7 @@ static int rhine_open(struct net_device *dev)
>        alloc_rbufs(dev);
>        alloc_tbufs(dev);
>        rhine_chip_reset(dev);
> +       rhine_task_enable(rp);
>        init_registers(dev);
>        if (debug > 2)
>                netdev_dbg(dev, "%s() Done - status %04x MII status: %04x\n",
> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - Dec. 30, 2011, 10:19 p.m.
Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
[...]
> Does not build with patch:

Neither did it here. My head is already elsewhere.

[...]
> Wouldn't it be an idea to move the declarations to a via-rhine.h file,
> and include it, to work around it, and seperate it like it should be?

Code could be correctly ordered too. Forward declare it. It's good enough
in the meantime.
Bjarke Istrup Pedersen - Dec. 30, 2011, 10:41 p.m.
2011/12/30 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Does not build with patch:
>
> Neither did it here. My head is already elsewhere.
>
> [...]
>> Wouldn't it be an idea to move the declarations to a via-rhine.h file,
>> and include it, to work around it, and seperate it like it should be?
>
> Code could be correctly ordered too. Forward declare it. It's good enough
> in the meantime.

Forward declaring it makes it compile just fine.

I'll try and do some stress test of it when the rest of the house goes
to sleep, so I don't cut off the net :)

/Bjarke

> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjarke Istrup Pedersen - Dec. 31, 2011, 1:37 a.m.
2011/12/30 Bjarke Istrup Pedersen <gurligebis@gentoo.org>:
> 2011/12/30 Francois Romieu <romieu@fr.zoreil.com>:
>> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
>> [...]
>>> Does not build with patch:
>>
>> Neither did it here. My head is already elsewhere.
>>
>> [...]
>>> Wouldn't it be an idea to move the declarations to a via-rhine.h file,
>>> and include it, to work around it, and seperate it like it should be?
>>
>> Code could be correctly ordered too. Forward declare it. It's good enough
>> in the meantime.
>
> Forward declaring it makes it compile just fine.
>
> I'll try and do some stress test of it when the rest of the house goes
> to sleep, so I don't cut off the net :)
>
> /Bjarke

Okay, this is beginning to look great.
Just tried randomly up'ing/down'ing the interfaces on the router while
there was alot of trafic going between them (a client downloading a
knoppix live dvd using bittorrent) - survived without a single bump.
Also tried connect a machine to one of the ports, and copying a large
file across (something that before could make it lock up within 15
secords) - also worked without any problems - CPU usage around 15%
(mostly "top" using that), so thats not bad at all.

Is there any other testcases that I should try out?

/Bjarke

>> --
>> Ueimor
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - Dec. 31, 2011, 12:17 p.m.
Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
[...]
> Also tried connect a machine to one of the ports, and copying a large
> file across (something that before could make it lock up within 15
> secords) - also worked without any problems - CPU usage around 15%
> (mostly "top" using that), so thats not bad at all.
> 
> Is there any other testcases that I should try out?

Same thing + random ethtool link management commands.
Same thing + random cable unplug/plug.
Same thing + pktgen facing the lan interface.

Everything at the same time.
Bjarke Istrup Pedersen - Jan. 1, 2012, 12:09 p.m.
2011/12/31 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Also tried connect a machine to one of the ports, and copying a large
>> file across (something that before could make it lock up within 15
>> secords) - also worked without any problems - CPU usage around 15%
>> (mostly "top" using that), so thats not bad at all.
>>
>> Is there any other testcases that I should try out?
>
> Same thing + random ethtool link management commands.
> Same thing + random cable unplug/plug.
> Same thing + pktgen facing the lan interface.
>
> Everything at the same time.

I tried messing around with plugging cables and playing with ethtool
at the same time, while there was alot of trafic, which went fine :)
Happy new year btw :)

/Bjarke

> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjarke Istrup Pedersen - Jan. 1, 2012, 9:15 p.m.
2011/12/31 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Also tried connect a machine to one of the ports, and copying a large
>> file across (something that before could make it lock up within 15
>> secords) - also worked without any problems - CPU usage around 15%
>> (mostly "top" using that), so thats not bad at all.
>>
>> Is there any other testcases that I should try out?
>
> Same thing + random ethtool link management commands.
> Same thing + random cable unplug/plug.
> Same thing + pktgen facing the lan interface.
>
> Everything at the same time.

Btw. - I have split up the via-rhine.c file (with all the changes you
have sent me) into via-rhine.c and via-rhine.h , like it should be :-)
I'll send it shortly after this mail - could you incorporate it into your work?

/Bjarke

> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjarke Istrup Pedersen - Jan. 3, 2012, 7:20 p.m.
2011/12/31 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Also tried connect a machine to one of the ports, and copying a large
>> file across (something that before could make it lock up within 15
>> secords) - also worked without any problems - CPU usage around 15%
>> (mostly "top" using that), so thats not bad at all.
>>
>> Is there any other testcases that I should try out?
>
> Same thing + random ethtool link management commands.
> Same thing + random cable unplug/plug.
> Same thing + pktgen facing the lan interface.
>
> Everything at the same time.

Hey,

Is there anything else I should try out?
Also, would it be possible to push these changes to mainline as
another CONFIG option (not sure if thats possible), so more people can
enable it and test it? :)

/Bjarke

> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - Jan. 3, 2012, 10:32 p.m.
Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
[...]
> Is there anything else I should try out?

Your computer is supposed to be moderately underpowered. Find someone
with an heavily overpowered one.

> Also, would it be possible to push these changes to mainline as

Probably. There has not been much objection nor comments so far.

> another CONFIG option (not sure if thats possible), so more people can
> enable it and test it? :)

I'd rather not. It would not be terribly elegant and more CONFIG options
would not imply more testing.
Bjarke Istrup Pedersen - Jan. 4, 2012, 7:59 a.m.
2012/1/3 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Is there anything else I should try out?
>
> Your computer is supposed to be moderately underpowered. Find someone
> with an heavily overpowered one.

Hmm, thats gonna be difficult, since it's an on-board chip in the box,
so it's no possible to move it to something more powerful :)

>> Also, would it be possible to push these changes to mainline as
>
> Probably. There has not been much objection nor comments so far.

Nice, any objections David? :)

>> another CONFIG option (not sure if thats possible), so more people can
>> enable it and test it? :)
>
> I'd rather not. It would not be terribly elegant and more CONFIG options
> would not imply more testing.

I agree, but if we push these changes to mainline directly, there is
no need - it was more as a way of getting it to mainline, while still
keeping the old way around, if it was needed.

/Bjarke

> --
> Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjarke Istrup Pedersen - Jan. 5, 2012, 12:01 p.m.
2012/1/4 Bjarke Istrup Pedersen <gurligebis@gentoo.org>:
> 2012/1/3 Francois Romieu <romieu@fr.zoreil.com>:
>> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
>> [...]
>>> Is there anything else I should try out?
>>
>> Your computer is supposed to be moderately underpowered. Find someone
>> with an heavily overpowered one.
>
> Hmm, thats gonna be difficult, since it's an on-board chip in the box,
> so it's no possible to move it to something more powerful :)
>
>>> Also, would it be possible to push these changes to mainline as
>>
>> Probably. There has not been much objection nor comments so far.
>
> Nice, any objections David? :)

Sorry for repeating myself David, but would it be possible to get this
into net-next for 3.4 inclusion?
(I'm thinking that now is the right time for it to have a complete
release cycle in net-next before inclusion) :)

/Bjarke

>>> another CONFIG option (not sure if thats possible), so more people can
>>> enable it and test it? :)
>>
>> I'd rather not. It would not be terribly elegant and more CONFIG options
>> would not imply more testing.
>
> I agree, but if we push these changes to mainline directly, there is
> no need - it was more as a way of getting it to mainline, while still
> keeping the old way around, if it was needed.
>
> /Bjarke
>
>> --
>> Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - Jan. 5, 2012, 4:20 p.m.
Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
[...]
> Sorry for repeating myself David, but would it be possible to get this
> into net-next for 3.4 inclusion ?

You should not expect #net-next for 3.4 to open before the merge window
for 3.3 is closed.

With due consideration for his freedom of will and action, David can not
take the current patchkit because he needs a formal submission of a complete
set where #3 is fixed. Our last friday's exchange does not count as one.

I understand it is important for you, it will be sent, ok ?

For future reference, you can see what is going on here :

http://patchwork.ozlabs.org/project/netdev/list/?state=*&q=via-rhine

/me returns to paperwork...
Bjarke Istrup Pedersen - Jan. 5, 2012, 4:46 p.m.
2012/1/5 Francois Romieu <romieu@fr.zoreil.com>:
> Bjarke Istrup Pedersen <gurligebis@gentoo.org> :
> [...]
>> Sorry for repeating myself David, but would it be possible to get this
>> into net-next for 3.4 inclusion ?
>
> You should not expect #net-next for 3.4 to open before the merge window
> for 3.3 is closed.

Okay, I'm still pretty new to how the kernel development model works,
and what opens when and for how long, but hey, you learn something
everyday :)

> With due consideration for his freedom of will and action, David can not
> take the current patchkit because he needs a formal submission of a complete
> set where #3 is fixed. Our last friday's exchange does not count as one.

Sure, it wasn't meant like that (Sorry if it was written that way, can
be difficult from time to time to get it across on paper) :)

> I understand it is important for you, it will be sent, ok ?

Yep :)

/Bjarke

> For future reference, you can see what is going on here :
>
> http://patchwork.ozlabs.org/project/netdev/list/?state=*&q=via-rhine
>
> /me returns to paperwork...
>
> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 5, 2012, 5:12 p.m.
From: Bjarke Istrup Pedersen <gurligebis@gentoo.org>
Date: Thu, 5 Jan 2012 13:01:41 +0100

> Sorry for repeating myself David, but would it be possible to get this
> into net-next for 3.4 inclusion?

Francois will formally submit it to me when he feels it is
appropriate, please be patient.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index c57e1da..89ced1b 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1536,6 +1536,7 @@  static int rhine_open(struct net_device *dev)
 	alloc_rbufs(dev);
 	alloc_tbufs(dev);
 	rhine_chip_reset(dev);
+	rhine_task_enable(rp);
 	init_registers(dev);
 	if (debug > 2)
 		netdev_dbg(dev, "%s() Done - status %04x MII status: %04x\n",