Skip to contentSkip to conversation

NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

PATCH New Jim Lin 3 participants 6 messages Jul 4, 2013 – Jul 10, 2013 Networking

lore
Share & tools
lore

Jim Lin

author
#0 lore raw

PATCH v5 1/1 NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.
This fix is to change Ctrl-C polling to every second when NET transfer is running.

Signed-off-by Jim Lin <[email protected]>

---
Changes in v2:
1. Change configuration name from CONFIG_CTRLC_POLL_MS to CONFIG_CTRLC_POLL_S.
2. New code will be executed only when CONFIG_CTRLC_POLL_S is defined in configuration header file.
3. Add description in README.console.
Changes in v3:
1. Move changes to common/usb_kbd.c and doc/README.usb
2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD.
3. Remove slow response on USB-keyboard input when TFTP boot is not running.
Changes in v4:
1. Remove changes in doc/README.usb, common/usb_kbd.c and CONFIG_USBKB_TESTC_PERIOD
2. Modify net/net.c
Changes in v5:
1. Change variable name to ctrlc_t_start.
2. Use two calls of get_timer(0) to get time gap.

 net/net.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
net/net.c +22 -0
index df94789..ec88b02 100644
@@ -322,6 +322,11 @@ int NetLoop(enum proto_t protocol)
 {
 	bd_t *bd = gd->bd;
 	int ret = -1;
+#ifdef CONFIG_USB_KEYBOARD
+	unsigned long ctrlc_t_start;
+	unsigned long ctrlc_t;
+	int ctrlc_result;
+#endif
 
 	NetRestarted = 0;
 	NetDevExists = 0;

@@ -472,7 +477,24 @@ restart:
 		/*
 		 *	Abort if ctrl-c was pressed.
 		 */
+#ifdef CONFIG_USB_KEYBOARD
+		/*
+		 * Reduce ctrl-c checking to 1 second once
+		 * to improve TFTP boot performance.
+		 */
+		if (ctrlc_t_start > get_timer(0))
+			ctrlc_t_start = get_timer(0);
+		ctrlc_t = get_timer(0) - ctrlc_t_start;
+		if (ctrlc_t > CONFIG_SYS_HZ) {
+			ctrlc_result = ctrlc();
+			ctrlc_t_start = get_timer(0);
+		} else {
+			ctrlc_result = 0;
+		}
+		if (ctrlc_result) {
+#else
 		if (ctrlc()) {
+#endif
 			/* cancel any ARP that may not have completed */
 			NetArpWaitPacketIP = 0;
Signature · 1 line
1.7.7
#1 lore raw

PATCH v5 1/1 NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

Quoted 4 lines · Jim Lin

TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.
This fix is to change Ctrl-C polling to every second when NET transfer is running.

Tested-by Stephen Warren <[email protected]>
#2 lore raw

PATCH v5 1/1 NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

In reply to Jim Lin
Quoted 4 lines · Jim Lin

TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.
This fix is to change Ctrl-C polling to every second when NET transfer is running.

Building this generates:

net.c: In function ?NetLoop?:
net.c:486:6: warning: ?ctrlc_t_start? may be used uninitialized in this function [-Wmaybe-uninitialized]

#3 lore raw

PATCH v5 1/1 NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

In reply to Jim Lin

Hi Jim and Stephen,

Quoted 44 lines · Jim Lin

TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.
This fix is to change Ctrl-C polling to every second when NET transfer is running.

Signed-off-by Jim Lin <[email protected]>

---
Changes in v2:
1. Change configuration name from CONFIG_CTRLC_POLL_MS to CONFIG_CTRLC_POLL_S.
2. New code will be executed only when CONFIG_CTRLC_POLL_S is defined in configuration header file.
3. Add description in README.console.
Changes in v3:
1. Move changes to common/usb_kbd.c and doc/README.usb
2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD.
3. Remove slow response on USB-keyboard input when TFTP boot is not running.
Changes in v4:
1. Remove changes in doc/README.usb, common/usb_kbd.c and CONFIG_USBKB_TESTC_PERIOD
2. Modify net/net.c
Changes in v5:
1. Change variable name to ctrlc_t_start.
2. Use two calls of get_timer(0) to get time gap.

 net/net.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
net/net.c +5 -0
index df94789..ec88b02 100644
@@ -322,6 +322,11 @@ int NetLoop(enum proto_t protocol)
 {
        bd_t *bd = gd->bd;
        int ret = -1;
+#ifdef CONFIG_USB_KEYBOARD
+       unsigned long ctrlc_t_start;
+       unsigned long ctrlc_t;
+       int ctrlc_result;
+#endif
 
        NetRestarted = 0;
        NetDevExists = 0;

net/net.c restart: · line 480

Quoted 4 lines
net/net.c +1 -0
@@ -472,7 +477,24 @@ restart:
                /*
                 *      Abort if ctrl-c was pressed.
                 */
+#ifdef CONFIG_USB_KEYBOARD

It seems this is the result of the USB Keyboard behavior. Why is it a good idea to litter the TFTP code with this unrelated code? It seems the very same check could be down inside of ctrlc() somewhere that is at least console I/O related. Besides, having it in a common place will allow any operation that accesses the keyboard to benefit from not hanging up on slow USB stuff.

It also seems that it should depend on what the actual source of the stdin is, not just if you compiled in CONFIG_USB_KEYBOARD support. Again, something that belongs in the console source.

net/net.c restart: · line 487

Quoted 7 lines
net/net.c +7 -0
+               /*
+                * Reduce ctrl-c checking to 1 second once
+                * to improve TFTP boot performance.
+                */
+               if (ctrlc_t_start > get_timer(0))
+                       ctrlc_t_start = get_timer(0);
+               ctrlc_t = get_timer(0) - ctrlc_t_start;

Why is it preferable to do the subtraction yourself instead of letting get_timer() do it? I.e. what compelled did you change this from v4?

net/net.c restart: · line 488

Quoted 1 line
net/net.c +1 -0
+               if (ctrlc_t > CONFIG_SYS_HZ) {

Why is hard-coding it to 1 second a good idea? Is that really how unresponsive it has to be to not significantly impact TFTP boot time?

Quoted 15 lines
net/net.c +8 -1
+                       ctrlc_result = ctrlc();
+                       ctrlc_t_start = get_timer(0);
+               } else {
+                       ctrlc_result = 0;
+               }
+               if (ctrlc_result) {
+#else
                if (ctrlc()) {
+#endif
                        /* cancel any ARP that may not have completed */
                        NetArpWaitPacketIP = 0;
 
--

1.7.7

author
#4 lore raw

PATCH v5 1/1 NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

Quoted 44 lines · Joe Hershberger

Hi Jim and Stephen,

Quoted 41 lines · Jim Lin

TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.
This fix is to change Ctrl-C polling to every second when NET transfer is running.

Signed-off-by Jim Lin <[email protected]>

---
Changes in v2:
1. Change configuration name from CONFIG_CTRLC_POLL_MS to CONFIG_CTRLC_POLL_S.
2. New code will be executed only when CONFIG_CTRLC_POLL_S is defined in configuration header file.
3. Add description in README.console.
Changes in v3:
1. Move changes to common/usb_kbd.c and doc/README.usb
2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD.
3. Remove slow response on USB-keyboard input when TFTP boot is not running.
Changes in v4:
1. Remove changes in doc/README.usb, common/usb_kbd.c and CONFIG_USBKB_TESTC_PERIOD
2. Modify net/net.c
Changes in v5:
1. Change variable name to ctrlc_t_start.
2. Use two calls of get_timer(0) to get time gap.

 net/net.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
net/net.c +5 -0
index df94789..ec88b02 100644
@@ -322,6 +322,11 @@ int NetLoop(enum proto_t protocol)
 {
        bd_t *bd = gd->bd;
        int ret = -1;
+#ifdef CONFIG_USB_KEYBOARD
+       unsigned long ctrlc_t_start;
+       unsigned long ctrlc_t;
+       int ctrlc_result;
+#endif
Quoted 10 lines

net/net.c restart: · line 480

Quoted 7 lines
net/net.c +1 -0
        NetRestarted = 0;
        NetDevExists = 0;

@@ -472,7 +477,24 @@ restart:
                /*
                 *      Abort if ctrl-c was pressed.
                 */
+#ifdef CONFIG_USB_KEYBOARD

It seems this is the result of the USB Keyboard behavior. Why is it a good idea to litter the TFTP code with this unrelated code? It seems

So far this is the best place to resolve this issue.

Quoted 7 lines

the very same check could be down inside of ctrlc() somewhere that is at least console I/O related. Besides, having it in a common place will allow any operation that accesses the keyboard to benefit from not hanging up on slow USB stuff.

It also seems that it should depend on what the actual source of the stdin is, not just if you compiled in CONFIG_USB_KEYBOARD support.

This issue only goes with USB keyboard installed and CONFIG_USB_KEYBOARD defined. Therefore compiled in CONFIG_USB_KEYBOARD support. Non-usb-keyboard doesn't have such issue.

Quoted 10 lines

Again, something that belongs in the console source.

net/net.c restart: · line 487

Quoted 7 lines
net/net.c +7 -0
+               /*
+                * Reduce ctrl-c checking to 1 second once
+                * to improve TFTP boot performance.
+                */
+               if (ctrlc_t_start > get_timer(0))
+                       ctrlc_t_start = get_timer(0);
+               ctrlc_t = get_timer(0) - ctrlc_t_start;
Quoted 2 lines

Why is it preferable to do the subtraction yourself instead of letting get_timer() do it? I.e. what compelled did you change this from v4?

As Wolfgang Denk said in another mail,
"An exception is "arch/arm/cpu/sa1100/timer.c" which does not respect the "base" argument at all, i. e. which is broken.
".

So this v5 patch uses get_timer(0), like other code did in this file.

Quoted 6 lines

net/net.c restart: · line 488

Quoted 2 lines
net/net.c +1 -0
+               if (ctrlc_t > CONFIG_SYS_HZ) {

Why is hard-coding it to 1 second a good idea?
Is that really how unresponsive it has to be to not significantly impact TFTP boot time?

Do you want me to add a CONFIG setting to have this time adjustable? I was thinking "1 second" checking on Ctrl-C should be fine while TFT boot is running.

--
nvpublic

#5 lore raw

PATCH v5 1/1 NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

Quoted 34 lines · Jim Lin
Quoted 22 lines · Joe Hershberger

Hi Jim and Stephen,

Quoted 19 lines · Jim Lin

TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.
This fix is to change Ctrl-C polling to every second when NET transfer is running.

net/net.c +5 -0
index df94789..ec88b02 100644
@@ -322,6 +322,11 @@ int NetLoop(enum proto_t protocol)
 {
        bd_t *bd = gd->bd;
        int ret = -1;
+#ifdef CONFIG_USB_KEYBOARD
+       unsigned long ctrlc_t_start;
+       unsigned long ctrlc_t;
+       int ctrlc_result;
+#endif
Quoted 10 lines

net/net.c restart: · line 480

Quoted 7 lines
net/net.c +1 -0
        NetRestarted = 0;
        NetDevExists = 0;

@@ -472,7 +477,24 @@ restart:
                /*
                 *      Abort if ctrl-c was pressed.
                 */
+#ifdef CONFIG_USB_KEYBOARD

It seems this is the result of the USB Keyboard behavior. Why is it a good idea to litter the TFTP code with this unrelated code? It seems

Quoted 1 line

So far this is the best place to resolve this issue.

I'm not convinced; the previous versions of the patch modified the USB keyboard driver, which seems a much better place to put the bulk of the code.

Now, it may be a good idea to have other code, such as the network loop, set a flag to tell the USB keyboard code (or other code) when some more performance-sensitive/real-time operation is going on with CTRL-C polling active, so that the USB keyboard code knows when to rate-limit its activity.

Quoted 11 lines
Quoted 7 lines

the very same check could be down inside of ctrlc() somewhere that is at least console I/O related. Besides, having it in a common place will allow any operation that accesses the keyboard to benefit from not hanging up on slow USB stuff.

It also seems that it should depend on what the actual source of the stdin is, not just if you compiled in CONFIG_USB_KEYBOARD support.

This issue only goes with USB keyboard installed and CONFIG_USB_KEYBOARD defined. Therefore compiled in CONFIG_USB_KEYBOARD support. Non-usb-keyboard doesn't have such issue.

Joe's point is that this new code should not be activated when "USB keyboard support is compiled in", but rather when "USB keyboard support is actively in use". The difference is that simply because CONFIG_USB_KEYBOARD is set does not imply that the USB keyboard must be used; the stdin environment variable need not always contain "usbkbd". (In fact, removing that string from stdin is how I work around this issue for now...)

Quoted 10 lines
Quoted 10 lines

Again, something that belongs in the console source.

Quoted 7 lines
net/net.c +7 -0
+               /*
+                * Reduce ctrl-c checking to 1 second once
+                * to improve TFTP boot performance.
+                */
+               if (ctrlc_t_start > get_timer(0))
+                       ctrlc_t_start = get_timer(0);
+               ctrlc_t = get_timer(0) - ctrlc_t_start;
Quoted 8 lines
Quoted 2 lines

Why is it preferable to do the subtraction yourself instead of letting get_timer() do it? I.e. what compelled did you change this from v4?

As Wolfgang Denk said in another mail,
"An exception is "arch/arm/cpu/sa1100/timer.c" which does not respect the "base" argument at all, i. e. which is broken.".

So this v5 patch uses get_timer(0), like other code did in this file.

That's a bug in the sa1100 timer code. The sa1100 timer code should be fixed; other code shouldn't have to work around it being broken.

Quoted 9 lines
Quoted 5 lines

net/net.c restart: · line 488

Quoted 1 line
net/net.c +1 -0
+               if (ctrlc_t > CONFIG_SYS_HZ) {

Why is hard-coding it to 1 second a good idea?
Is that really how unresponsive it has to be to not significantly impact TFTP boot time?

Do you want me to add a CONFIG setting to have this time adjustable? I was thinking "1 second" checking on Ctrl-C should be fine while TFT boot is running.

1s polling seems fine to me. If someone else wants something different, presumably they can add a config option for it?