Re: [PATCH] Firewire node manager causes up to ~3.25s delay in freezing tasks.

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Nigel Cunningham wrote:
> Hi all.
> 
> (Okay, having learnt that ieee1394 != bluetooth, I'll try again :>)
> 
> The firewire nodemanager function "nodemgr_host_thread" contains a loop
> that calls try_to_freeze near the top of the loop, but then delays for
> up to 3.25 seconds (plus time to do work) before getting back to the top
> of the loop. When starting a cycle post-boot, this doesn't seem to bite,
> but it is causing a noticeable delay at boot time, when freezing
> processes prior to starting to read the image.

In 2.6.27 and less, these were only 0.25 seconds.  The 3 additional
seconds in 2.6.28-rc are my doing and sort of a regression.  It occurs
to me that I should send this patch to mainline before release.

> The following patch adds invocation of try_to_freeze to the subloops
> that are used in the body of this function. With these additions, the
> time to freeze when starting to resume at boot time is virtually zero.
> I'm no expert on bluetooth, and so don't know that we shouldn't check
> the return value and jump back to the top of the loop or such like after
> being frozen, but I submit it for your consideration.
> 
> Signed-off-by: Nigel Cunningham <nigel@tuxonice.net>

As far as I can tell, try_to_freeze() without any jump is correct.

>  nodemgr.c |    2 ++
>  1 file changed, 2 insertions(+)
> diff -ruNp 710-nodemanager-freezing.patch-old/drivers/ieee1394/nodemgr.c 710-nodemanager-freezing.patch-new/drivers/ieee1394/nodemgr.c
> --- 710-nodemanager-freezing.patch-old/drivers/ieee1394/nodemgr.c	2008-12-06 08:42:08.000000000 +1100
> +++ 710-nodemanager-freezing.patch-new/drivers/ieee1394/nodemgr.c	2008-12-09 18:49:26.000000000 +1100
> @@ -1685,6 +1685,7 @@ static int nodemgr_host_thread(void *dat
>  		g = get_hpsb_generation(host);
>  		for (i = 0; i < 4 ; i++) {
>  			msleep_interruptible(63);
> +			try_to_freeze();
>  			if (kthread_should_stop())
>  				goto exit;
>  

Here we have the goal to proceed only >= 0.25s after the last bus reset
happened.  While the new try_to_freeze() held up nodemgr, another bus
reset may happen.  But we check for this later in the loop and handle
it, so this is OK.

> @@ -1725,6 +1726,7 @@ static int nodemgr_host_thread(void *dat
>  		/* Sleep 3 seconds */
>  		for (i = 3000/200; i; i--) {
>  			msleep_interruptible(200);
> +			try_to_freeze();
>  			if (kthread_should_stop())
>  				goto exit;
>  

Ditto, the rest of the loop catches the case that another bus reset
occurred during the sleep or freeze.

Thanks for what appears to be a regression fix,
-- 
Stefan Richter
-=====-==--- ==-- -=--=
http://arcgraph.de/sr/