Re: [Iscsitarget-devel] Worker Thread Pool

[Arne R. <arn...@go...>]

Hi Andreas,

Am Sonntag, den 25.01.2009, 16:51 +0100 schrieb Andreas Florath:
> Hello Arne,
> 
> many thanks for the style hints - I really have some problems with
> them, because typically I'm using the converse (like don't use spaces
> where there are not needed) - but you're completly rigth to insists on
> them.

No problem. I really recommend looking at the
$KERNELSRC/Documentation/CodingStyle, it's at least an entertaining
read.

> 
> +    if ((err = wthread_start(worker_thread_pool, worker_thread_pool_size,
> 0)) < 0) {
> +        kfree(worker_thread_pool);
> 
> > worker_thread_pool should be set to NULL here, so it's not kfree()'d
> > again in wthread_module_exit().
> 
> Yep - did not think about this case.  (Also in C++, which I normally
> use, a destructor is not called when the constructor fails :-). )
> 
> I have some comments (look at the five ':')
> 
> Index: kernel/wthread.c
> ===================================================================
> --- kernel/wthread.c    (revision 200)
> +++ kernel/wthread.c    (working copy)
> @@ -9,9 +9,11 @@
>  #include "iscsi.h"
>  #include "iscsi_dbg.h"
>  
> +struct worker_thread_info *worker_thread_pool;
> 
> ::::: Here I'm not sure about the removed '= NULL'.  In 'normal'
> ::::: programs such a pointer is not initialised at all,
> ::::: i.e. it can contain random data.

No, variables with global and file scope (i.e. static) are always
initialized to 0. See http://c-faq.com/decl/initval.html

> ::::: Typically the test 'if (worker_thread_pool)' means, that the
> ::::: thread pool exists. (Example: see kernel/param.c line 129)
> ::::: I do not know, if this initialisation is different in kernel
> ::::: modules.
>
> +
> +int wthread_module_init()
> +{
> +    int err;
> +
> +    if (!worker_thread_pool_size)
> +        return 0;
> +
> +    worker_thread_pool = kmalloc(sizeof(struct worker_thread_info),
> +                     GFP_KERNEL);
> +    if (!worker_thread_pool)
> +        return -ENOMEM;
> +
> +    wthread_init(worker_thread_pool);
> +
> +    err = wthread_start(worker_thread_pool, worker_thread_pool_size, 0);
> +    if (err) {
> 
> ::::: My original proposal was:
> :::::   if ((err = wthread_start([...])) < 0) {
> ::::: which is mostly the same as is used in target.cc line 103
> ::::: (Version 202):
> :::::     if ((err = wthread_start(target)) < 0) {
> ::::: Is there a reason for changing the check?

Coding style: assignments in an if () or while () tend to be error
prone. The original code does that though, but then again it's not that
big an issue to go over it and fix all occurrences. but since the patch
touches an occurrence I figured I clean this one up.

> 
> +        kfree(worker_thread_pool);
> +        worker_thread_pool = NULL;
> +        return err;
> +    }
> +
> +    iprintk("iscsi_trgt using worker thread pool; size = %ld\n",
> +        worker_thread_pool_size);
> 
> ::::: My suggestion was a 'printk' instead a 'iprintk', because the
> ::::: other initial log messages also used printk, like
> ::::: printk("iSCSI Enterprise Target Software - version %s\n",
> :::::   IET_VERSION_STRING);
> ::::: from iscsi.c

The other one should be converted to iprintk() instead, because besides
adding a nice "iscsi_trgt" prefix it also explicitly sets the log level
(to KERN_INFO, which is more appropriate than the default KERN_WARNING).

> +
> +    return 0;
> +}
> +
> 
> I'll start some test tomorrow morning with your changes applied and
> give it 24h full load using the thread pool and another 24h using the
> threads-per-target.  Will send the results on Wednesday or Thursday.

Great, looking forward to it.

Thanks,
Arne