Re: [PATCH 2/2] pcrypt: sysfs interface

[Steffen Klassert]

On Tue, Jun 29, 2010 at 08:39:39PM +0400, Dan Kruchinin wrote:

[snip]

> @@ -390,6 +427,9 @@ static int __init pcrypt_init(void)
>  	if (!pcrypt_dec_padata)
>  		goto err_free_padata;
>  
> +	if (pcrypt_init_sysfs() != 0)
> +		goto err_free_padata;

We leak pcrypt_dec_padata if pcrypt_init_sysfs() fails.
pcrypt_dec_padata must be freed in this case.

[snip]

>  struct padata_priv {
> -	struct list_head	list;
> -	struct parallel_data	*pd;
> -	int			cb_cpu;
> -	int			seq_nr;
> -	int			info;
> -	void                    (*parallel)(struct padata_priv *padata);
> -	void                    (*serial)(struct padata_priv *padata);
> +	struct list_head      list;
> +	struct parallel_data *pd;
> +	int                   cb_cpu;
> +	int                   seq_nr;
> +	int                   info;
> +	void                  (*parallel)(struct padata_priv *padata);
> +	void                  (*serial)(struct padata_priv *padata);
>  };

These whitespace changes are quite pointless.
Please try to avoid such whitespace changes in patches that
change real content. 

[snip]

> @@ -107,6 +108,7 @@ static void padata_parallel_worker(struct work_struct *parallel_work)
>  				    struct padata_priv, list);
>  
>  		list_del_init(&padata->list);
> +		atomic_dec(&pinst->stat.parallel_objs);
>  
>  		padata->parallel(padata);
>  	}
> @@ -166,6 +168,7 @@ int padata_do_parallel(struct padata_instance *pinst,
>  	pqueue = per_cpu_ptr(pd->pqueue, target_cpu);
>  
>  	spin_lock(&pqueue->parallel.lock);
> +	atomic_inc(&pinst->stat.parallel_objs);
>  	list_add_tail(&padata->list, &pqueue->parallel.list);
>  	spin_unlock(&pqueue->parallel.lock);

These statistic counters add a lot of atomic operations to the fast-path.
Would'nt it be better to have these statistics in a percpu manner?
This would avoid the atomic operations and we would get some additional
information on the distribution of the queued objects.

>  
> @@ -202,6 +205,7 @@ static struct padata_priv *padata_get_next(struct parallel_data *pd)
>  	struct padata_parallel_queue *queue, *next_queue;
>  	struct padata_priv *padata;
>  	struct padata_list *reorder;
> +	struct padata_stat *stat = &pd->pinst->stat;
>  
>  	empty = 0;
>  	next_nr = -1;
> @@ -266,7 +270,7 @@ static struct padata_priv *padata_get_next(struct parallel_data *pd)
>  
>  		spin_lock(&reorder->lock);
>  		list_del_init(&padata->list);
> -		atomic_dec(&pd->reorder_objects);
> +		atomic_dec(&stat->reorder_objs);
>  		spin_unlock(&reorder->lock);
>  
>  		atomic_inc(&next_queue->num_obj);
> @@ -330,6 +334,7 @@ static void padata_reorder(struct parallel_data *pd)
>  		squeue = per_cpu_ptr(pd->squeue, padata->cb_cpu);
>  
>  		spin_lock(&squeue->serial.lock);
> +        atomic_inc(&pinst->stat.serial_objs);

Wrong code indent.

Please try to split the patch in a padata and a pcrypt part.
I.e. add the interface to padata, then use it with pcrypt.

Thanks again,

Steffen
--
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/