Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread Patrick McHardy
Florian Fainelli wrote:
 diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
 index 87d2046..fdc5a8a 100644
 --- a/drivers/leds/Kconfig
 +++ b/drivers/leds/Kconfig
 @@ -128,5 +128,12 @@ config LEDS_TRIGGER_HEARTBEAT
 load average.
 If unsure, say Y.
  
 +config LEDS_TRIGGER_NETWORK_ACT
 + tristate LED Network Activity Trigger


Module isn't possible, you call the led trigger from net/core/dev.c.


 diff --git a/net/core/dev.c b/net/core/dev.c
 index ee051bb..a3a4115 100644
 --- a/net/core/dev.c
 +++ b/net/core/dev.c
 @@ -117,6 +117,7 @@
  #include linux/err.h
  #include linux/ctype.h
  #include linux/if_arp.h
 +#include linux/leds.h
  
  /*
   *   The list of packet types we will receive (as opposed to discard)
 @@ -1523,6 +1524,7 @@ gso:
* stops preemption for RCU.
*/
   rcu_read_lock_bh();
 + ledtrig_network_activity();


Besides missing a declaration and not linking without the network
LED config option, its pretty ridiculous to call this for every
packet just to make a led blink.

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


Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread Stephen Hemminger
On Wed, 18 Jul 2007 15:27:38 +0200
Florian Fainelli [EMAIL PROTECTED] wrote:

 Hi all,
 
 This patch adds a new LED trigger, based on network activity. It gathers 
 activity from net/core/dev.c and can be used as a LED trigger by 
 specifying network-activity. Further version should allow the user to 
 specify the network interface to bind a LED to. This trigger is a simple 
 trigger as defined by the LED subsystem.
 
 Signed-off-by: Florian Fainelli [EMAIL PROTECTED]
 -- 

You are slowing down the network receive path even if LED is not used.

Doing mod_timer() is expensive, on a busy system you would be adding
another lock roundtrip and list operation for each incoming packet.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread Florian Fainelli
Hello Patrick,

Le mercredi 18 juillet 2007, Patrick McHardy a écrit :
 Module isn't possible, you call the led trigger from net/core/dev.c.

You are right, it just occured to me.

 Besides missing a declaration and not linking without the network
 LED config option, its pretty ridiculous to call this for every
 packet just to make a led blink.

Could you suggest me a better way to do so ? The code was highly inspired from 
what is done with the IDE trigger. The declaration is done in linux/leds.h, 
which is included in dev.c for that purpose.

Thank you very much for your answer.


signature.asc
Description: This is a digitally signed message part.


Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread Patrick McHardy

Florian Fainelli wrote:

Besides missing a declaration and not linking without the network
LED config option, its pretty ridiculous to call this for every
packet just to make a led blink.



Could you suggest me a better way to do so ? The code was highly inspired from 
what is done with the IDE trigger. The declaration is done in linux/leds.h, 
which is included in dev.c for that purpose.
  


Maybe just increment a variable and periodically check it or something
like that.


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


Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread Richard Purdie
On Wed, 2007-07-18 at 15:54 +0200, Patrick McHardy wrote:
 Florian Fainelli wrote:
  Besides missing a declaration and not linking without the network
  LED config option, its pretty ridiculous to call this for every
  packet just to make a led blink.
  
 
  Could you suggest me a better way to do so ? The code was highly inspired 
  from 
  what is done with the IDE trigger. The declaration is done in linux/leds.h, 
  which is included in dev.c for that purpose.

 
 Maybe just increment a variable and periodically check it or something
 like that.

Are there not already packet counters that the LED trigger could just
look at? If it did that at say 20Hz or 10Hz, it would probably look
quite reasonable without impacting on the system too much?

Richard

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


Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread Patrick McHardy

Richard Purdie wrote:

On Wed, 2007-07-18 at 15:54 +0200, Patrick McHardy wrote:
  

Florian Fainelli wrote:


Besides missing a declaration and not linking without the network
LED config option, its pretty ridiculous to call this for every
packet just to make a led blink.


Could you suggest me a better way to do so ? The code was highly inspired from 
what is done with the IDE trigger. The declaration is done in linux/leds.h, 
which is included in dev.c for that purpose.
  
  

Maybe just increment a variable and periodically check it or something
like that.



Are there not already packet counters that the LED trigger could just
look at? If it did that at say 20Hz or 10Hz, it would probably look
quite reasonable without impacting on the system too much


The qdisc counters might work, I think all qdiscs properly maintain
at least byte and packet counters. Or simply check the queue length,
if  0 there is activity.

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


Re: [PATCH] Add a network activity LED trigger

2007-07-18 Thread David Miller
From: Patrick McHardy [EMAIL PROTECTED]
Date: Wed, 18 Jul 2007 15:54:56 +0200

 Florian Fainelli wrote:
  Besides missing a declaration and not linking without the network
  LED config option, its pretty ridiculous to call this for every
  packet just to make a led blink.
  
 
  Could you suggest me a better way to do so ? The code was highly inspired 
  from 
  what is done with the IDE trigger. The declaration is done in linux/leds.h, 
  which is included in dev.c for that purpose.

 
 Maybe just increment a variable and periodically check it or something
 like that.

Yes please, calling this millions of times per second on a high end
router is just not smart.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html