[PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi Herbert and Co,

This series adds a struct crypto_alg->cra_check_optimized() that can be optionally
used by crc32c.ko (and other algorithms) in order to make a request_module() call
to load architecture dependent offload code for a libcrypto base algorithm.

This patch is a result of feedback during review for the iscsi_target_mod fabric
module here:

Re: [RFC 12/12] iscsi-target: Add Makefile/Kconfig and update TCM
http://marc.info/?l=linux-scsi&m=129974455726437&w=2

Please review and comment,

Thanks!

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

Nicholas Bellinger (2):
  crypto: Add struct crypto_alg->cra_check_optimized
  crypto/crc32c: Add crc32c_cra_check_optimized for crc32c_intel

 crypto/api.c           |    6 +++++-
 crypto/crc32c.c        |   23 +++++++++++++++++++++++
 include/linux/crypto.h |    1 +
 3 files changed, 29 insertions(+), 1 deletions(-)

[PATCH 1/2] crypto: Add struct crypto_alg->cra_check_optimized

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds an optional struct crypto_alg->cra_check_optimized() caller
that can be used by libcrypto algorithms in order to load an architecture
dependent optimized / HW offload module.

This includes adding the call to alg->cra_check_optimized() inside of
crypto_larval_lookup() once the initial generic algorithm has been
loaded via request_module().

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/api.c           |    6 +++++-
 include/linux/crypto.h |    1 +
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/crypto/api.c b/crypto/api.c
index 033a714..4dcbef6 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -226,8 +226,12 @@ struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
 		alg = crypto_alg_lookup(name, type, mask);
 	}
 
-	if (alg)
+	if (alg) {
+		if (alg->cra_check_optimized)
+			alg->cra_check_optimized(alg);
+
 		return crypto_is_larval(alg) ? crypto_larval_wait(alg) : alg;
+	}
 
 	return crypto_larval_add(name, type, mask);
 }
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index a6a7a1c..ea7c426 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -295,6 +295,7 @@ struct crypto_alg {
 
 	int (*cra_init)(struct crypto_tfm *tfm);
 	void (*cra_exit)(struct crypto_tfm *tfm);
+	void (*cra_check_optimized)(struct crypto_alg *alg);
 	void (*cra_destroy)(struct crypto_alg *alg);
 	
 	struct module *cra_module;
-- 
1.5.6.5

[PATCH 2/2] crypto/crc32c: Add crc32c_cra_check_optimized for crc32c_intel

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds crc32c_cra_check_optimized() for use by the new
struct crypto_alg->cra_check_optimized() API caller in order to
call request_module() to load crc32c_intel.ko for CRC32C instruction
offload when available on CONFIG_X86 w/ SSE v4.2 compatiable hardware.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/crc32c.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/crypto/crc32c.c b/crypto/crc32c.c
index de9e55c..3f1d3a3 100644
--- a/crypto/crc32c.c
+++ b/crypto/crc32c.c
@@ -44,6 +44,8 @@
 #define CHKSUM_BLOCK_SIZE	1
 #define CHKSUM_DIGEST_SIZE	4
 
+static int crc32c_offload_enabled;
+
 struct chksum_ctx {
 	u32 key;
 };
@@ -221,6 +223,24 @@ static int crc32c_cra_init(struct crypto_tfm *tfm)
 	return 0;
 }
 
+static void crc32c_cra_check_optimized(struct crypto_alg *alg)
+{
+	if (crc32c_offload_enabled)
+		return;
+#ifdef CONFIG_X86
+	/*
+	 * For cpu_has_xmm4_2 go ahead and load crc32c_intel.ko in order so
+	 * crypto_alloc_hash("crc32c", ...) will automatically use SSE v4.2
+	 * CRC32C instruction offload.
+	 */
+	if (cpu_has_xmm4_2) {
+		int rc = request_module("crc32c_intel");
+		if (rc == 0)
+			crc32c_offload_enabled = 1;
+	}
+#endif
+}
+
 static struct shash_alg alg = {
 	.digestsize		=	CHKSUM_DIGEST_SIZE,
 	.setkey			=	chksum_setkey,
@@ -239,11 +259,14 @@ static struct shash_alg alg = {
 		.cra_ctxsize		=	sizeof(struct chksum_ctx),
 		.cra_module		=	THIS_MODULE,
 		.cra_init		=	crc32c_cra_init,
+		.cra_check_optimized	=	crc32c_cra_check_optimized,
 	}
 };
 
 static int __init crc32c_mod_init(void)
 {
+	crc32c_offload_enabled = 0;
+
 	return crypto_register_shash(&alg);
 }
 
-- 
1.5.6.5

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Herbert Xu]

On Thu, Mar 10, 2011 at 12:21:10AM -0800, Nicholas A. Bellinger wrote:
> 
> Nicholas Bellinger (2):
>   crypto: Add struct crypto_alg->cra_check_optimized
>   crypto/crc32c: Add crc32c_cra_check_optimized for crc32c_intel

I think you misunderstood my suggestion.  The reason I asked
for this to be done in the crypto API is because it is not specific
to crc32c_intel.

So sending me a patch that modifies crc32c clearly misses the
point :)

This should be done in the core API, i.e., api.c/algapi.c.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Nicholas A. Bellinger]

On Thu, 2011-03-10 at 16:43 +0800, Herbert Xu wrote:
> On Thu, Mar 10, 2011 at 12:21:10AM -0800, Nicholas A. Bellinger wrote:
> > 
> > Nicholas Bellinger (2):
> >   crypto: Add struct crypto_alg->cra_check_optimized
> >   crypto/crc32c: Add crc32c_cra_check_optimized for crc32c_intel
> 
> I think you misunderstood my suggestion.  The reason I asked
> for this to be done in the crypto API is because it is not specific
> to crc32c_intel.
> 
> So sending me a patch that modifies crc32c clearly misses the
> point :)
> 
> This should be done in the core API, i.e., api.c/algapi.c.
> 

OK, so you mean each struct crypto_alg should define something like a
'cra_optimized_name' for which request_module(alg->cra_optimized_name)
is called somewhere in libcrypto code..?

Would you mind being a bit more specific where this should be happening
in api.c/algapi.c..?

Thanks,

--nab

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Herbert Xu]

On Thu, Mar 10, 2011 at 12:54:24AM -0800, Nicholas A. Bellinger wrote:
>
> OK, so you mean each struct crypto_alg should define something like a
> 'cra_optimized_name' for which request_module(alg->cra_optimized_name)
> is called somewhere in libcrypto code..?

No, what I mean is that whenever we look up an algorithm through
crypto_alg_mod_lookup, we should conditionally call modprobe if
we havn't done so already.

So you just need to record one bit of info in each crypto_alg
object to indicate whether we have invoked modprobe.  I suggest
adding a CRYPTO_ALG_* bit.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Nicholas A. Bellinger]

On Thu, 2011-03-10 at 17:09 +0800, Herbert Xu wrote:
> On Thu, Mar 10, 2011 at 12:54:24AM -0800, Nicholas A. Bellinger wrote:
> >
> > OK, so you mean each struct crypto_alg should define something like a
> > 'cra_optimized_name' for which request_module(alg->cra_optimized_name)
> > is called somewhere in libcrypto code..?
> 
> No, what I mean is that whenever we look up an algorithm through
> crypto_alg_mod_lookup, we should conditionally call modprobe if
> we havn't done so already.
> 
> So you just need to record one bit of info in each crypto_alg
> object to indicate whether we have invoked modprobe.  I suggest
> adding a CRYPTO_ALG_* bit.
> 

Mmmm, now I am really confused, and please let me apologize in advance
for my lack of experience with libcrypto internals..  ;)

I thought the problem was that CONFIG_CRYPTO_ALGFOO=y and
CONFIG_CRYPTO_ALGFOO_ARCH_HW_OFFLOAD=m would cause the latter to not
explictly call request_module() for this HW offload case..

So what I don't understand how adding a request_module() call to a list
of known modules works when crc32c_intel.ko has not been loaded yet..?

Am I missing something obvious wrt to how crc32c.ko can tell libcrypto
about which architecture dependent optimized modules it should load..?

Best Regards,

--nab

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Nicholas A. Bellinger]

On Thu, 2011-03-10 at 01:13 -0800, Nicholas A. Bellinger wrote:
> On Thu, 2011-03-10 at 17:09 +0800, Herbert Xu wrote:
> > On Thu, Mar 10, 2011 at 12:54:24AM -0800, Nicholas A. Bellinger wrote:
> > >
> > > OK, so you mean each struct crypto_alg should define something like a
> > > 'cra_optimized_name' for which request_module(alg->cra_optimized_name)
> > > is called somewhere in libcrypto code..?
> > 
> > No, what I mean is that whenever we look up an algorithm through
> > crypto_alg_mod_lookup, we should conditionally call modprobe if
> > we havn't done so already.
> > 
> > So you just need to record one bit of info in each crypto_alg
> > object to indicate whether we have invoked modprobe.  I suggest
> > adding a CRYPTO_ALG_* bit.
> > 
> 
> Mmmm, now I am really confused, and please let me apologize in advance
> for my lack of experience with libcrypto internals..  ;)
> 
> I thought the problem was that CONFIG_CRYPTO_ALGFOO=y and
> CONFIG_CRYPTO_ALGFOO_ARCH_HW_OFFLOAD=m would cause the latter to not
> explictly call request_module() for this HW offload case..
> 
> So what I don't understand how adding a request_module() call to a list
> of known modules works when crc32c_intel.ko has not been loaded yet..?
> 
> Am I missing something obvious wrt to how crc32c.ko can tell libcrypto
> about which architecture dependent optimized modules it should load..?
> 

Just to clarify a bit on my previous comments..

We are still expecting the libcrypto consumer (iscsi_target_mod.ko) to
call the arch independent crypto_alloc_hash("crc32c", ...) in order to
have libcrypto backend logic perform a request_module() upon
architecture dependent offload modules (like crc32c_intel.ko) that
libcrypto consumers are not (and should not) be calling directly via
crypto_alloc_host("crc32c_intel", ...), correct..?

Where I am getting confused is wrt to a new crypto_alg_mod_lookup() ->
request_module() call for a struct shash_alg that has not yet be loaded
via arch/x86/crypto/crc32c-intel.c:crc32c_intel_mod_init() ->
crypto_register_shash().

Thanks,

--nab

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Herbert Xu]

On Thu, Mar 10, 2011 at 02:05:17AM -0800, Nicholas A. Bellinger wrote:
>
> We are still expecting the libcrypto consumer (iscsi_target_mod.ko) to
> call the arch independent crypto_alloc_hash("crc32c", ...) in order to
> have libcrypto backend logic perform a request_module() upon
> architecture dependent offload modules (like crc32c_intel.ko) that
> libcrypto consumers are not (and should not) be calling directly via
> crypto_alloc_host("crc32c_intel", ...), correct..?

Right.

> Where I am getting confused is wrt to a new crypto_alg_mod_lookup() ->
> request_module() call for a struct shash_alg that has not yet be loaded
> via arch/x86/crypto/crc32c-intel.c:crc32c_intel_mod_init() ->
> crypto_register_shash().

If you look at crypto_alg_mod_lookup, basically there are two paths.
Either we already have a registered algorithm of the requested name,
or we don't.

In the first case, we won't invoke request_module and in the second
case we will.

So what I'm suggesting is that in the first case we also invoke
request_module conditionally.  Now exactly what that condition is
is the tricky bit.

The easiest is to flip a bit in the algorithm we just found.  This
isn't optimal as it'll mean that for each unregistered algorithm
we'll end up modprobing twice, but that shouldn't be too bad I
think.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/2] Add struct crypto_alg->cra_check_optimized for crc32c_intel

[Nicholas A. Bellinger]

On Sun, 2011-03-13 at 17:01 +0800, Herbert Xu wrote:
> On Thu, Mar 10, 2011 at 02:05:17AM -0800, Nicholas A. Bellinger wrote:
> >
> > We are still expecting the libcrypto consumer (iscsi_target_mod.ko) to
> > call the arch independent crypto_alloc_hash("crc32c", ...) in order to
> > have libcrypto backend logic perform a request_module() upon
> > architecture dependent offload modules (like crc32c_intel.ko) that
> > libcrypto consumers are not (and should not) be calling directly via
> > crypto_alloc_host("crc32c_intel", ...), correct..?
> 
> Right.
> 
> > Where I am getting confused is wrt to a new crypto_alg_mod_lookup() ->
> > request_module() call for a struct shash_alg that has not yet be loaded
> > via arch/x86/crypto/crc32c-intel.c:crc32c_intel_mod_init() ->
> > crypto_register_shash().
> 
> If you look at crypto_alg_mod_lookup, basically there are two paths.
> Either we already have a registered algorithm of the requested name,
> or we don't.
> 
> In the first case, we won't invoke request_module and in the second
> case we will.
> 
> So what I'm suggesting is that in the first case we also invoke
> request_module conditionally.  Now exactly what that condition is
> is the tricky bit.
> 
> The easiest is to flip a bit in the algorithm we just found.  This
> isn't optimal as it'll mean that for each unregistered algorithm
> we'll end up modprobing twice, but that shouldn't be too bad I
> think.
> 

Hi Herbert,

Thanks for your feedback on this issue, and again my apologies for the
lack of experience beyond the external libcrypto API.  I will take
another look this week and see if something can be produced more along
the lines of what you had in mind to resolve this special case.  In the
mean time the extra crypto_alloc_hash("crc32c-intel", ...) calls have
been removed from RFC-v2 iscsi-target code posted earlier this morning.

Please feel free to add any other pointers and I will have a look.

Best Regards,

--nab