[Crash-utility] [PATCH 3/4] remove unreachable (and slow) code

[Dave Anderson]

----- Original Message -----
> task_exists() scans known tasks for a given task address.
> 
> refresh_radix_tree_task_table() looks like:
>   hq_open()
>   for cpus {
>     hq_enter(per_cpu_idle_thread)
>   }
>   for ... {
>     hq_enter(task)
>   }
>   hq_close()
> 
>   tt->running_tasks = 0
>   for task in retrieve_list(hq_entries) {
>     if (task_exists()) {
>        duplicate task address
>        continue
>     }
>     add_context()
>   }
> 
> Because retrieve_list returns unique tasks, the above task_exists()
> check will never find a duplicate task.  So remove the check.  This
> converts the above loop from O(N^2) to O(N) which saves considerable
> startup time when there are a large number of tasks.  On a 1M task dump
> this patch reduces startup time: 44m => 1m.
> 
> I was not able to test the other pid table routines, but I suspect they
> may also benefit from similar removal.

Hi Greg,

I'm thinking that the task_exists() removal may be the most beneficial
piece of the patch set, right?

I have a set of ~250 vmcore files gathered over the years, but only 2 of
them are from kernels using the idr radix tree scheme.  I timed how long
it took to simply bring up all 250 vmcores to their crash prompt and then
quit immediately.  These are the total times running with and without your
patch:

without the patch:

  real	46m44.023s
  user	17m29.956s
  sys	0m58.940s

with your patch applied:

  real	46m39.752s
  user	17m36.496s
  sys	0m58.269s

But as you've noted, given that all of the various refresh_xxx functions 
use hq_enter(), the task_exists() call is pretty much useless.  

I'm going to remove the task_exists() call from the remaining refresh_xxx
functions, rerun the test, and see how it affects the numbers.  I suspect
it will help a bit, although the task count in the ~250 vmcores range from
22 to 4178, with only ~20 dumpfiles having more than 1000 tasks.

Thanks,
  Dave






> 
> Signed-off-by: Greg Thelen <gthelen at google.com>
> ---
>  task.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/task.c b/task.c
> index 82fa015805cb..cddc1f5b651a 100644
> --- a/task.c
> +++ b/task.c
> @@ -2473,16 +2473,6 @@ retry_radix_tree:
>  			goto retry_radix_tree;
>  		}
>  
> -		if (task_exists(*tlp)) {
> -			error(WARNING,
> -		           "%sduplicate task address found in task list: %lx\n",
> -				DUMPFILE() ? "\n" : "", *tlp);
> -			if (DUMPFILE())
> -				continue;
> -			retries++;
> -			goto retry_radix_tree;
> -		}
> -
>  		if (!(tp = fill_task_struct(*tlp))) {
>  			if (DUMPFILE())
>  				continue;
> --
> 2.17.0.484.g0c8726318c-goog
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>