Re: [PATCH v3 1/2] crypto: Implement a generic crypto statistics
From: LABBE Corentin
Date: Sun Nov 04 2018 - 04:11:37 EST
On Sat, Nov 03, 2018 at 03:19:36PM -0700, Eric Biggers wrote:
> Hi Corentin,
>
> On Wed, Sep 19, 2018 at 10:10:54AM +0000, Corentin Labbe wrote:
> > This patch implement a generic way to get statistics about all crypto
> > usages.
> >
> > Signed-off-by: Corentin Labbe <clabbe@xxxxxxxxxxxx>
> > ---
> > crypto/Kconfig | 11 +
> > crypto/Makefile | 1 +
> > crypto/ahash.c | 21 +-
> > crypto/algapi.c | 8 +
> > crypto/{crypto_user.c => crypto_user_base.c} | 9 +-
> > crypto/crypto_user_stat.c | 463 +++++++++++++++++++++++++++
> > crypto/rng.c | 1 +
> > include/crypto/acompress.h | 38 ++-
> > include/crypto/aead.h | 51 ++-
> > include/crypto/akcipher.h | 76 ++++-
> > include/crypto/hash.h | 32 +-
> > include/crypto/internal/cryptouser.h | 8 +
> > include/crypto/kpp.h | 51 ++-
> > include/crypto/rng.h | 29 +-
> > include/crypto/skcipher.h | 44 ++-
> > include/linux/crypto.h | 110 ++++++-
> > include/uapi/linux/cryptouser.h | 52 +++
> > 17 files changed, 970 insertions(+), 35 deletions(-)
> > rename crypto/{crypto_user.c => crypto_user_base.c} (97%)
> > create mode 100644 crypto/crypto_user_stat.c
> > create mode 100644 include/crypto/internal/cryptouser.h
> >
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index 90f2811fac5f..4ef95b0b25a3 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -1799,6 +1799,17 @@ config CRYPTO_USER_API_AEAD
> > This option enables the user-spaces interface for AEAD
> > cipher algorithms.
> >
> > +config CRYPTO_STATS
> > + bool "Crypto usage statistics for User-space"
> > + help
> > + This option enables the gathering of crypto stats.
> > + This will collect:
> > + - encrypt/decrypt size and numbers of symmeric operations
> > + - compress/decompress size and numbers of compress operations
> > + - size and numbers of hash operations
> > + - encrypt/decrypt/sign/verify numbers for asymmetric operations
> > + - generate/seed numbers for rng operations
> > +
> > config CRYPTO_HASH_INFO
> > bool
> >
> > diff --git a/crypto/Makefile b/crypto/Makefile
> > index d719843f8b6e..ff5c2bbda04a 100644
> > --- a/crypto/Makefile
> > +++ b/crypto/Makefile
> > @@ -54,6 +54,7 @@ cryptomgr-y := algboss.o testmgr.o
> >
> > obj-$(CONFIG_CRYPTO_MANAGER2) += cryptomgr.o
> > obj-$(CONFIG_CRYPTO_USER) += crypto_user.o
> > +crypto_user-y := crypto_user_base.o crypto_user_stat.o
> > obj-$(CONFIG_CRYPTO_CMAC) += cmac.o
> > obj-$(CONFIG_CRYPTO_HMAC) += hmac.o
> > obj-$(CONFIG_CRYPTO_VMAC) += vmac.o
>
> Why is crypto_user_stat.c always being compiled when CONFIG_CRYPTO_USER is
> enabled, even when CONFIG_CRYPTO_STATS is not?
>
> When CONFIG_CRYPTO_STATS is disabled, all the crypto stat stuff should be
> stubbed out so that crypto_user_stat.c doesn't need to be compiled.
>
> [...]
> > diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> > index e8839d3a7559..3634ad6fe202 100644
> > --- a/include/linux/crypto.h
> > +++ b/include/linux/crypto.h
> > @@ -454,6 +454,33 @@ struct compress_alg {
> > * @cra_refcnt: internally used
> > * @cra_destroy: internally used
> > *
> > + * All following statistics are for this crypto_alg
> > + * @encrypt_cnt: number of encrypt requests
> > + * @decrypt_cnt: number of decrypt requests
> > + * @compress_cnt: number of compress requests
> > + * @decompress_cnt: number of decompress requests
> > + * @generate_cnt: number of RNG generate requests
> > + * @seed_cnt: number of times the rng was seeded
> > + * @hash_cnt: number of hash requests
> > + * @sign_cnt: number of sign requests
> > + * @setsecret_cnt: number of setsecrey operation
> > + * @generate_public_key_cnt: number of generate_public_key operation
> > + * @verify_cnt: number of verify operation
> > + * @compute_shared_secret_cnt: number of compute_shared_secret operation
> > + * @encrypt_tlen: total data size handled by encrypt requests
> > + * @decrypt_tlen: total data size handled by decrypt requests
> > + * @compress_tlen: total data size handled by compress requests
> > + * @decompress_tlen: total data size handled by decompress requests
> > + * @generate_tlen: total data size of generated data by the RNG
> > + * @hash_tlen: total data size hashed
> > + * @akcipher_err_cnt: number of error for akcipher requests
> > + * @cipher_err_cnt: number of error for akcipher requests
> > + * @compress_err_cnt: number of error for akcipher requests
> > + * @aead_err_cnt: number of error for akcipher requests
> > + * @hash_err_cnt: number of error for akcipher requests
> > + * @rng_err_cnt: number of error for akcipher requests
> > + * @kpp_err_cnt: number of error for akcipher requests
> > + *
> > * The struct crypto_alg describes a generic Crypto API algorithm and is common
> > * for all of the transformations. Any variable not documented here shall not
> > * be used by a cipher implementation as it is internal to the Crypto API.
> > @@ -487,6 +514,45 @@ struct crypto_alg {
> > void (*cra_destroy)(struct crypto_alg *alg);
> >
> > struct module *cra_module;
> > +
> > + union {
> > + atomic_t encrypt_cnt;
> > + atomic_t compress_cnt;
> > + atomic_t generate_cnt;
> > + atomic_t hash_cnt;
> > + atomic_t setsecret_cnt;
> > + };
> > + union {
> > + atomic64_t encrypt_tlen;
> > + atomic64_t compress_tlen;
> > + atomic64_t generate_tlen;
> > + atomic64_t hash_tlen;
> > + };
> > + union {
> > + atomic_t akcipher_err_cnt;
> > + atomic_t cipher_err_cnt;
> > + atomic_t compress_err_cnt;
> > + atomic_t aead_err_cnt;
> > + atomic_t hash_err_cnt;
> > + atomic_t rng_err_cnt;
> > + atomic_t kpp_err_cnt;
> > + };
> > + union {
> > + atomic_t decrypt_cnt;
> > + atomic_t decompress_cnt;
> > + atomic_t seed_cnt;
> > + atomic_t generate_public_key_cnt;
> > + };
> > + union {
> > + atomic64_t decrypt_tlen;
> > + atomic64_t decompress_tlen;
> > + };
> > + union {
> > + atomic_t verify_cnt;
> > + atomic_t compute_shared_secret_cnt;
> > + };
> > + atomic_t sign_cnt;
> > +
> > } CRYPTO_MINALIGN_ATTR;
>
> The new fields here should all be behind CONFIG_CRYPTO_STATS.
>
> Ideally there would be zero overhead when CONFIG_CRYPTO_STATS is disabled.
> That's not quite the case currently.
>
> - Eric
Hello
I will address thoses problem in a followup patch.
Thanks