pcrypt: sysfs interface

PATCH In review Dan Kruchinin 3 participants 6 messages Jun 29, 2010 – Jul 5, 2010 1 message not in the archive

Subsystems (3)

Crypto API Padata parallel execution mechanism Pcrypt parallel crypto engine

lore

Share & tools

lore

Dan Kruchinin

author

2010-06-29 16:39 UTC #0 lore raw

PATCH 2/2 pcrypt: sysfs interface

Sysfs interface for pcrypt: /sys/kernel/pcrypt/pencrypt and /sys/kernel/pcrypt/pencrypt padata_instance now contains a kobject with common operations that may be embedded by the user to a kset or can be made a child of any other kobject.
Padata sysfs interface adds the following files:
serial_cpumask [RW] - cpumask for callback CPUs and hence serial workers parallel_cpumask [RW] - cpumask for parallel workers
serial_objecs [RO] - number of objects waiting for serialization
reorder_objects [RO] - number of objects waiting in reorder queues parallel_objects [RO] - number of objects waiting for parallelization

crypto/pcrypt.c +41 -0

index 20f3e47..ec146b2 100644
@@ -24,12 +24,14 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/kobject.h>
 #include <crypto/pcrypt.h>
 
 static struct padata_instance *pcrypt_enc_padata;
 static struct padata_instance *pcrypt_dec_padata;
 static struct workqueue_struct *encwq;
 static struct workqueue_struct *decwq;
+static struct kset             *pcrypt_kset;
 
 struct pcrypt_instance_ctx {
 	struct crypto_spawn spawn;

@@ -364,6 +366,41 @@ static void pcrypt_free(struct crypto_instance *inst)
 	kfree(inst);
 }
 
+static int pcrypt_sysfs_add(struct padata_instance *pinst, const char *name)
+{
+    int ret;
+
+    pinst->kobj.kset = pcrypt_kset;
+    ret = kobject_add(&pinst->kobj, NULL, name);
+    if (!ret)
+        kobject_uevent(&pinst->kobj, KOBJ_ADD);
+
+    return ret;
+}
+
+static int pcrypt_init_sysfs(void)
+{
+    int ret;
+
+    pcrypt_kset = kset_create_and_add("pcrypt", NULL, kernel_kobj);
+    if (!pcrypt_kset)
+        return -ENOMEM;
+
+    ret = pcrypt_sysfs_add(pcrypt_enc_padata, "pencrypt");
+    if (ret)
+        goto err;
+
+    ret = pcrypt_sysfs_add(pcrypt_dec_padata, "pdecrypt");
+    if (ret)
+        goto err;
+
+    return ret;
+
+err:
+    kset_unregister(pcrypt_kset);
+    return ret;
+}
+
 static struct crypto_template pcrypt_tmpl = {
 	.name = "pcrypt",
 	.alloc = pcrypt_alloc,

@@ -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;
+
 	padata_start(pcrypt_enc_padata);
 	padata_start(pcrypt_dec_padata);

@@ -416,6 +456,7 @@ static void __exit pcrypt_exit(void)
 	destroy_workqueue(encwq);
 	destroy_workqueue(decwq);
 
+	kset_unregister(pcrypt_kset);
 	padata_free(pcrypt_enc_padata);
 	padata_free(pcrypt_dec_padata);

include/linux/padata.h +31 ~~-13~~

83 lines not on this page · raw message

kernel/padata.c +192 ~~-21~~

326 lines not on this page · raw message

Signature · 2 lines

W.B.R.
Dan Kruchinin

Steffen Klassert

maintainer

2010-06-30 12:46 UTC #1 lore raw

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

[snip]

crypto/pcrypt.c static int __init pcrypt_init(void) · line 431

Quoted 6 lines

crypto/pcrypt.c +2 -0

@@ -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]

Quoted 6 lines

include/linux/padata.h +0 -5

 struct padata_priv {
-	struct list_head	list;
-	struct parallel_data	*pd;
-	int			cb_cpu;
-	int			seq_nr;
-	int			info;

include/linux/padata.h line 52

Quoted 10 lines

include/linux/padata.h +7 -2

-	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]

Quoted 9 lines

kernel/padata.c +1 -0

@@ -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);
 	}

kernel/padata.c int padata_do_parallel(struct padata_instance *pinst, · line 173

Quoted 6 lines

kernel/padata.c +1 -0

@@ -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.

Quoted 19 lines

kernel/padata.c +2 -1

@@ -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);

kernel/padata.c static void padata_reorder(struct parallel_data *pd) · line 337

Quoted 4 lines

kernel/padata.c +1 -0

@@ -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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Steffen Klassert

maintainer

2010-07-02 09:06 UTC #3 lore raw

Quoted 17 lines · Dan Kruchinin

Quoted 6 lines
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.
If I understood you correctly the resulting sysfs hierarchy would look like this one:
pcrypt/
|- serial_cpumask
|- parallel_cpumask
|- w0/
+--- parallel_objects
+--- serial_objects
+--- reorder_objects
|- w1/

Quoted 5 lines

...
|- wN/
right? If so I think it won't be very convenient to monitor summary number of parallel, serial and reorder objects.

Yes, I thought about something like this. You can still take the sum over the percpu objects when you output the statistics.

Quoted 2 lines

Anyway I think these atomic operations take very small time in comparison with other operations in padata. So small that it can be ignored.

I have a patch in queue that simplifies the serialization mechanism and reduces the accesses of foreign and global memory as much as possible in the parallel codepath. Adding atomic operations to global memory (just to collect statistics) to the parallel codepath would go in the opposite direction.

Steffen

Dan Kruchinin

2010-07-02 10:20 UTC #4 lore raw

Quoted 21 lines · Steffen Klassert

Quoted 20 lines · Dan Kruchinin
Quoted 6 lines
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.
If I understood you correctly the resulting sysfs hierarchy would look like this one:
pcrypt/
|- serial_cpumask
|- parallel_cpumask
|- w0/
+--- parallel_objects
+--- serial_objects
+--- reorder_objects
|- w1/
...
|- wN/

Quoted 5 lines

Quoted 2 lines
right? If so I think it won't be very convenient to monitor summary number of parallel, serial and reorder objects.
Yes, I thought about something like this. You can still take the sum over the percpu objects when you output the statistics.

But summation can not be clear without some kind of lock because while we're summing another CPU can increase or decrease its percpu statistic counters. Then each statistic percpu counter must be modified under lock, right?

Quoted 13 lines

Quoted 4 lines
Anyway I think these atomic operations take very small time in comparison with other operations in padata. So small that it can be ignored.
I have a patch in queue that simplifies the serialization mechanism and reduces the accesses of foreign and global memory as much as possible in the parallel codepath. Adding atomic operations to global memory (just to collect statistics) to the parallel codepath would go in the opposite direction.
Steffen

Signature · 2 lines

W.B.R.
Dan Kruchinin

Steffen Klassert

maintainer

2010-07-02 11:20 UTC #5 lore raw

Quoted 8 lines · Dan Kruchinin

Quoted 3 lines
Yes, I thought about something like this. You can still take the sum over the percpu objects when you output the statistics.
But summation can not be clear without some kind of lock because while we're summing another CPU can increase or decrease its percpu statistic counters. Then each statistic percpu counter must be modified under lock, right?

Yes, the counters must accessed under lock. In the fastpath functions you hold the appropriate lock anyway. Modifying a local percpu value should not be too painfull there.

The expensive thing is to access the percpu statistics, but this happens on demand and is probaply a rare event.

Steffen

Steffen Klassert

maintainer

2010-07-05 11:10 UTC #6 lore raw

In reply to Dan Kruchinin

Quoted 26 lines · Dan Kruchinin

Quoted 24 lines · Steffen Klassert
Quoted 22 lines · Dan Kruchinin
Quoted 6 lines
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.
If I understood you correctly the resulting sysfs hierarchy would look like this one:
pcrypt/
|- serial_cpumask
|- parallel_cpumask
|- w0/
+--- parallel_objects
+--- serial_objects
+--- reorder_objects
|- w1/
...
|- wN/
right? If so I think it won't be very convenient to monitor summary number of parallel, serial and reorder objects.

Quoted 7 lines

Quoted 2 lines
Yes, I thought about something like this. You can still take the sum over the percpu objects when you output the statistics.
But summation can not be clear without some kind of lock because while we're summing another CPU can increase or decrease its percpu statistic counters. Then each statistic percpu counter must be modified under lock, right?

Thinking a bit longer about this statistics, this statistics work should be an extra patch. We should focus on the cpumask separation now and think about this statistics later.

Steffen