Re: [PATCH 01/45] Create a dynamically sized pool of threads for doing very slow work items [ver #41]

[David Howells]

Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> This may be the most skilfully commented kernel code I've ever seen.

Thanks.  I like useful comments as I may need to re-understand the code I've
written should I have to fix it in five years time.

> > +static unsigned slow_work_min_threads = 2;
> > +static unsigned slow_work_max_threads = (NR_CPUS > 4) ? NR_CPUS : 4;
> 
> I suspect there will be a requirement to tune this at runtime.

Yes.  See:

	[PATCH 03/45] Make the slow work pool configurable [ver #41]

> Using num_possible_cpus() would be more accurate here.  One could
> easily envisage NR_CPUS=1024 on a 2-way machine.  Generally any use
> of NR_CPUS is a red flag.  In fact there's a checkpatch warning about
> it now.

This has been fixed in ver #42, which is what is in the GIT trees.  I can post
that as patches if you like.  Well #43, I've just noticed a bug:-(.

> > +#define slow_work_available(vsmax) \
> > +	(!list_empty(&slow_work_queue) || \
> > +	 (!list_empty(&vslow_work_queue) && \
> > +	  atomic_read(&vslow_work_executing_count) < (vsmax)))
> 
> This could be a regular C function?

I suppose so.

> > +			p = kthread_create(slow_work_thread, NULL,
> > +					   "kslow%Xd", loop);
> > +			if (!p)
> > +				goto error;
> > +			wake_up_process(p);
> 
> The above reimplements kthread_run().

And wrongly.  I should use IS_ERR(p) not !p.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/