[PATCH 0/3] crypto: allow users to specify acomp hardware from a desired NUMA node

[PATCH 0/3] crypto: allow users to specify acomp hardware from a desired NUMA node

[Barry Song]

For a typical Linux server, probably there are several hardware modules.
For example, numa node0 has a compressor, numa node2 has a same module.
Some drivers are automatically using the module near the CPU calling
acomp_alloc.
But it isn't necessarily correct. Just like memory allocation API like
kmalloc and kmalloc_node. Similar optimization may be done for crypto.

Barry Song (3):
  crypto: permit users to specify numa node of acomp hardware
  crypto: hisilicon/zip - permit users to specify NUMA node
  mm/zswap: specify the NUMA node of acomp to use local compressors
  [mm/zswap patch is on top of linux-next tree]

 crypto/acompress.c                        |  8 ++++++++
 crypto/api.c                              | 22 ++++++++++++++--------
 crypto/internal.h                         | 23 +++++++++++++++++++----
 drivers/crypto/hisilicon/zip/zip.h        |  2 +-
 drivers/crypto/hisilicon/zip/zip_crypto.c |  6 +++---
 drivers/crypto/hisilicon/zip/zip_main.c   |  5 +++--
 include/crypto/acompress.h                |  7 +++++++
 include/linux/crypto.h                    |  3 ++-
 mm/zswap.c                                |  2 +-
 9 files changed, 58 insertions(+), 20 deletions(-)

-- 
2.27.0

[PATCH 1/3] crypto: permit users to specify numa node of acomp hardware

[Barry Song]

For a Linux server with NUMA, there are possibly multiple (de)compressors
which are either local or remote to some NUMA node. Some drivers will
automatically use the (de)compressor near the CPU calling acomp_alloc().
However, it is not necessarily correct because users who send acomp_req
could be from different NUMA node with the CPU which allocates acomp.

Just like kernel has kmalloc() and kmalloc_node(), here crypto can have
same support.

Cc: Seth Jennings <sjenning@redhat.com>
Cc: Dan Streetman <ddstreet@ieee.org>
Cc: Vitaly Wool <vitaly.wool@konsulko.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 crypto/acompress.c         |  8 ++++++++
 crypto/api.c               | 22 ++++++++++++++--------
 crypto/internal.h          | 23 +++++++++++++++++++----
 include/crypto/acompress.h |  7 +++++++
 include/linux/crypto.h     |  3 ++-
 5 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/crypto/acompress.c b/crypto/acompress.c
index 84a76723e851..c32c72048a1c 100644
--- a/crypto/acompress.c
+++ b/crypto/acompress.c
@@ -109,6 +109,14 @@ struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32 type,
 }
 EXPORT_SYMBOL_GPL(crypto_alloc_acomp);
 
+struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name, u32 type,
+					u32 mask, int node)
+{
+	return crypto_alloc_tfm_node(alg_name, &crypto_acomp_type, type, mask,
+				node);
+}
+EXPORT_SYMBOL_GPL(crypto_alloc_acomp_node);
+
 struct acomp_req *acomp_request_alloc(struct crypto_acomp *acomp)
 {
 	struct crypto_tfm *tfm = crypto_acomp_tfm(acomp);
diff --git a/crypto/api.c b/crypto/api.c
index edcf690800d4..4ecf712286af 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -433,8 +433,9 @@ struct crypto_tfm *crypto_alloc_base(const char *alg_name, u32 type, u32 mask)
 }
 EXPORT_SYMBOL_GPL(crypto_alloc_base);
 
-void *crypto_create_tfm(struct crypto_alg *alg,
-			const struct crypto_type *frontend)
+void *crypto_create_tfm_node(struct crypto_alg *alg,
+			const struct crypto_type *frontend,
+			int node)
 {
 	char *mem;
 	struct crypto_tfm *tfm = NULL;
@@ -451,6 +452,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
 
 	tfm = (struct crypto_tfm *)(mem + tfmsize);
 	tfm->__crt_alg = alg;
+	tfm->node = node;
 
 	err = frontend->init_tfm(tfm);
 	if (err)
@@ -472,7 +474,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
 out:
 	return mem;
 }
-EXPORT_SYMBOL_GPL(crypto_create_tfm);
+EXPORT_SYMBOL_GPL(crypto_create_tfm_node);
 
 struct crypto_alg *crypto_find_alg(const char *alg_name,
 				   const struct crypto_type *frontend,
@@ -490,11 +492,13 @@ struct crypto_alg *crypto_find_alg(const char *alg_name,
 EXPORT_SYMBOL_GPL(crypto_find_alg);
 
 /*
- *	crypto_alloc_tfm - Locate algorithm and allocate transform
+ *	crypto_alloc_tfm_node - Locate algorithm and allocate transform
  *	@alg_name: Name of algorithm
  *	@frontend: Frontend algorithm type
  *	@type: Type of algorithm
  *	@mask: Mask for type comparison
+ *	@node: NUMA node in which users desire to put requests, if node is
+ *		NUMA_NO_NODE, it means users have no special requirement.
  *
  *	crypto_alloc_tfm() will first attempt to locate an already loaded
  *	algorithm.  If that fails and the kernel supports dynamically loadable
@@ -509,8 +513,10 @@ EXPORT_SYMBOL_GPL(crypto_find_alg);
  *
  *	In case of error the return value is an error pointer.
  */
-void *crypto_alloc_tfm(const char *alg_name,
-		       const struct crypto_type *frontend, u32 type, u32 mask)
+
+void *crypto_alloc_tfm_node(const char *alg_name,
+		       const struct crypto_type *frontend, u32 type, u32 mask,
+		       int node)
 {
 	void *tfm;
 	int err;
@@ -524,7 +530,7 @@ void *crypto_alloc_tfm(const char *alg_name,
 			goto err;
 		}
 
-		tfm = crypto_create_tfm(alg, frontend);
+		tfm = crypto_create_tfm_node(alg, frontend, node);
 		if (!IS_ERR(tfm))
 			return tfm;
 
@@ -542,7 +548,7 @@ void *crypto_alloc_tfm(const char *alg_name,
 
 	return ERR_PTR(err);
 }
-EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
+EXPORT_SYMBOL_GPL(crypto_alloc_tfm_node);
 
 /*
  *	crypto_destroy_tfm - Free crypto transform
diff --git a/crypto/internal.h b/crypto/internal.h
index ff06a3bd1ca1..1b92a5a61852 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -68,13 +68,28 @@ void crypto_remove_final(struct list_head *list);
 void crypto_shoot_alg(struct crypto_alg *alg);
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 				      u32 mask);
-void *crypto_create_tfm(struct crypto_alg *alg,
-			const struct crypto_type *frontend);
+void *crypto_create_tfm_node(struct crypto_alg *alg,
+			const struct crypto_type *frontend, int node);
+
+static inline void *crypto_create_tfm(struct crypto_alg *alg,
+			const struct crypto_type *frontend)
+{
+	return crypto_create_tfm_node(alg, frontend, NUMA_NO_NODE);
+}
+
 struct crypto_alg *crypto_find_alg(const char *alg_name,
 				   const struct crypto_type *frontend,
 				   u32 type, u32 mask);
-void *crypto_alloc_tfm(const char *alg_name,
-		       const struct crypto_type *frontend, u32 type, u32 mask);
+
+void *crypto_alloc_tfm_node(const char *alg_name,
+		       const struct crypto_type *frontend, u32 type, u32 mask,
+		       int node);
+
+static inline void *crypto_alloc_tfm(const char *alg_name,
+		       const struct crypto_type *frontend, u32 type, u32 mask)
+{
+	return crypto_alloc_tfm_node(alg_name, frontend, type, mask, NUMA_NO_NODE);
+}
 
 int crypto_probing_notify(unsigned long val, void *v);
 
diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
index 2b4d2b06ccbd..b1a78687014a 100644
--- a/include/crypto/acompress.h
+++ b/include/crypto/acompress.h
@@ -106,6 +106,13 @@ struct acomp_alg {
  */
 struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32 type,
 					u32 mask);
+/**
+ * crypto_alloc_acomp_node() -- allocate ACOMPRESS tfm handle with desired NUMA
+ *				node
+ * @node:	specifies the NUMA node the ZIP hardware belongs to
+ */
+struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name, u32 type,
+					u32 mask, int node);
 
 static inline struct crypto_tfm *crypto_acomp_tfm(struct crypto_acomp *tfm)
 {
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 763863dbc079..c1a47ce4c09e 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -593,8 +593,9 @@ int crypto_has_alg(const char *name, u32 type, u32 mask);
  */
 
 struct crypto_tfm {
-
 	u32 crt_flags;
+
+	int node;
 	
 	void (*exit)(struct crypto_tfm *tfm);
 	
-- 
2.27.0

[PATCH 2/3] crypto: hisilicon/zip - permit users to specify NUMA node

[Barry Song]

If users don't specify NUMA node, the driver will use the ZIP module near
the CPU allocating acomp. Otherwise, it uses the ZIP module according to
the requirement of users.

Cc: Zhou Wang <wangzhou1@hisilicon.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 drivers/crypto/hisilicon/zip/zip.h        | 2 +-
 drivers/crypto/hisilicon/zip/zip_crypto.c | 6 +++---
 drivers/crypto/hisilicon/zip/zip_main.c   | 5 +++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/hisilicon/zip/zip.h b/drivers/crypto/hisilicon/zip/zip.h
index f3ed4c0e5493..4484be13812b 100644
--- a/drivers/crypto/hisilicon/zip/zip.h
+++ b/drivers/crypto/hisilicon/zip/zip.h
@@ -76,7 +76,7 @@ struct hisi_zip_sqe {
 	u32 rsvd1[4];
 };
 
-int zip_create_qps(struct hisi_qp **qps, int ctx_num);
+int zip_create_qps(struct hisi_qp **qps, int ctx_num, int node);
 int hisi_zip_register_to_crypto(void);
 void hisi_zip_unregister_from_crypto(void);
 #endif
diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
index c73707c2e539..01fd6a78111d 100644
--- a/drivers/crypto/hisilicon/zip/zip_crypto.c
+++ b/drivers/crypto/hisilicon/zip/zip_crypto.c
@@ -158,13 +158,13 @@ static void hisi_zip_release_qp(struct hisi_zip_qp_ctx *ctx)
 	hisi_qm_release_qp(ctx->qp);
 }
 
-static int hisi_zip_ctx_init(struct hisi_zip_ctx *hisi_zip_ctx, u8 req_type)
+static int hisi_zip_ctx_init(struct hisi_zip_ctx *hisi_zip_ctx, u8 req_type, int node)
 {
 	struct hisi_qp *qps[HZIP_CTX_Q_NUM] = { NULL };
 	struct hisi_zip *hisi_zip;
 	int ret, i, j;
 
-	ret = zip_create_qps(qps, HZIP_CTX_Q_NUM);
+	ret = zip_create_qps(qps, HZIP_CTX_Q_NUM, node);
 	if (ret) {
 		pr_err("Can not create zip qps!\n");
 		return -ENODEV;
@@ -379,7 +379,7 @@ static int hisi_zip_acomp_init(struct crypto_acomp *tfm)
 	struct hisi_zip_ctx *ctx = crypto_tfm_ctx(&tfm->base);
 	int ret;
 
-	ret = hisi_zip_ctx_init(ctx, COMP_NAME_TO_TYPE(alg_name));
+	ret = hisi_zip_ctx_init(ctx, COMP_NAME_TO_TYPE(alg_name), tfm->base.node);
 	if (ret)
 		return ret;
 
diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
index 2229a21ae7c8..e2845b2c963d 100644
--- a/drivers/crypto/hisilicon/zip/zip_main.c
+++ b/drivers/crypto/hisilicon/zip/zip_main.c
@@ -234,9 +234,10 @@ static const struct pci_device_id hisi_zip_dev_ids[] = {
 };
 MODULE_DEVICE_TABLE(pci, hisi_zip_dev_ids);
 
-int zip_create_qps(struct hisi_qp **qps, int qp_num)
+int zip_create_qps(struct hisi_qp **qps, int qp_num, int node)
 {
-	int node = cpu_to_node(smp_processor_id());
+	if (node == NUMA_NO_NODE)
+		node = cpu_to_node(smp_processor_id());
 
 	return hisi_qm_alloc_qps_node(&zip_devices, qp_num, 0, node, qps);
 }
-- 
2.27.0

[PATCH 3/3] mm/zswap: specify the NUMA node of acomp to use local compressors

[Barry Song]

zswap_cpu_comp_prepare() is called on a different CPU with the CPU which
will really send acomp_req. In order to use the right local compressors,
this patch specifies the NUMA node to which the CPU sending acomp_req
belongs.

Cc: Seth Jennings <sjenning@redhat.com>
Cc: Dan Streetman <ddstreet@ieee.org>
Cc: Vitaly Wool <vitaly.wool@konsulko.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller" <davem@davemloft.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 mm/zswap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 0d914ba6b4a0..9b1aa477022e 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -437,7 +437,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 		pr_err("Could not initialize acomp_ctx\n");
 		return -ENOMEM;
 	}
-	acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
+	acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
 	if (IS_ERR_OR_NULL(acomp)) {
 		pr_err("could not alloc crypto acomp %s : %ld\n",
 				pool->tfm_name, PTR_ERR(acomp));
-- 
2.27.0

Re: [PATCH 1/3] crypto: permit users to specify numa node of acomp hardware

[Jonathan Cameron]

On Mon, 22 Jun 2020 14:48:59 +1200
Barry Song <song.bao.hua@hisilicon.com> wrote:

> For a Linux server with NUMA, there are possibly multiple (de)compressors
> which are either local or remote to some NUMA node. Some drivers will
> automatically use the (de)compressor near the CPU calling acomp_alloc().
> However, it is not necessarily correct because users who send acomp_req
> could be from different NUMA node with the CPU which allocates acomp.
> 
> Just like kernel has kmalloc() and kmalloc_node(), here crypto can have
> same support.
> 
> Cc: Seth Jennings <sjenning@redhat.com>
> Cc: Dan Streetman <ddstreet@ieee.org>
> Cc: Vitaly Wool <vitaly.wool@konsulko.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>

Hi Barry,

Seems sensible to me.  A few trivial comments inline.

Thanks,

Jonathan

> ---
>  crypto/acompress.c         |  8 ++++++++
>  crypto/api.c               | 22 ++++++++++++++--------
>  crypto/internal.h          | 23 +++++++++++++++++++----
>  include/crypto/acompress.h |  7 +++++++
>  include/linux/crypto.h     |  3 ++-
>  5 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/crypto/acompress.c b/crypto/acompress.c
> index 84a76723e851..c32c72048a1c 100644
> --- a/crypto/acompress.c
> +++ b/crypto/acompress.c
> @@ -109,6 +109,14 @@ struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32 type,
>  }
>  EXPORT_SYMBOL_GPL(crypto_alloc_acomp);
>  
> +struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name, u32 type,
> +					u32 mask, int node)
> +{
> +	return crypto_alloc_tfm_node(alg_name, &crypto_acomp_type, type, mask,
> +				node);
> +}
> +EXPORT_SYMBOL_GPL(crypto_alloc_acomp_node);
> +
>  struct acomp_req *acomp_request_alloc(struct crypto_acomp *acomp)
>  {
>  	struct crypto_tfm *tfm = crypto_acomp_tfm(acomp);
> diff --git a/crypto/api.c b/crypto/api.c
> index edcf690800d4..4ecf712286af 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -433,8 +433,9 @@ struct crypto_tfm *crypto_alloc_base(const char *alg_name, u32 type, u32 mask)
>  }
>  EXPORT_SYMBOL_GPL(crypto_alloc_base);
>  
> -void *crypto_create_tfm(struct crypto_alg *alg,
> -			const struct crypto_type *frontend)
> +void *crypto_create_tfm_node(struct crypto_alg *alg,
> +			const struct crypto_type *frontend,
> +			int node)
>  {
>  	char *mem;
>  	struct crypto_tfm *tfm = NULL;
> @@ -451,6 +452,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
>  
>  	tfm = (struct crypto_tfm *)(mem + tfmsize);
>  	tfm->__crt_alg = alg;
> +	tfm->node = node;
>  
>  	err = frontend->init_tfm(tfm);
>  	if (err)
> @@ -472,7 +474,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
>  out:
>  	return mem;
>  }
> -EXPORT_SYMBOL_GPL(crypto_create_tfm);
> +EXPORT_SYMBOL_GPL(crypto_create_tfm_node);
>  
>  struct crypto_alg *crypto_find_alg(const char *alg_name,
>  				   const struct crypto_type *frontend,
> @@ -490,11 +492,13 @@ struct crypto_alg *crypto_find_alg(const char *alg_name,
>  EXPORT_SYMBOL_GPL(crypto_find_alg);
>  
>  /*
> - *	crypto_alloc_tfm - Locate algorithm and allocate transform
> + *	crypto_alloc_tfm_node - Locate algorithm and allocate transform
>   *	@alg_name: Name of algorithm
>   *	@frontend: Frontend algorithm type
>   *	@type: Type of algorithm
>   *	@mask: Mask for type comparison
> + *	@node: NUMA node in which users desire to put requests, if node is
> + *		NUMA_NO_NODE, it means users have no special requirement.
>   *
>   *	crypto_alloc_tfm() will first attempt to locate an already loaded
>   *	algorithm.  If that fails and the kernel supports dynamically loadable
> @@ -509,8 +513,10 @@ EXPORT_SYMBOL_GPL(crypto_find_alg);
>   *
>   *	In case of error the return value is an error pointer.
>   */
> -void *crypto_alloc_tfm(const char *alg_name,
> -		       const struct crypto_type *frontend, u32 type, u32 mask)
> +
> +void *crypto_alloc_tfm_node(const char *alg_name,
> +		       const struct crypto_type *frontend, u32 type, u32 mask,
> +		       int node)
>  {
>  	void *tfm;
>  	int err;
> @@ -524,7 +530,7 @@ void *crypto_alloc_tfm(const char *alg_name,
>  			goto err;
>  		}
>  
> -		tfm = crypto_create_tfm(alg, frontend);
> +		tfm = crypto_create_tfm_node(alg, frontend, node);
>  		if (!IS_ERR(tfm))
>  			return tfm;
>  
> @@ -542,7 +548,7 @@ void *crypto_alloc_tfm(const char *alg_name,
>  
>  	return ERR_PTR(err);
>  }
> -EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
> +EXPORT_SYMBOL_GPL(crypto_alloc_tfm_node);
>  
>  /*
>   *	crypto_destroy_tfm - Free crypto transform
> diff --git a/crypto/internal.h b/crypto/internal.h
> index ff06a3bd1ca1..1b92a5a61852 100644
> --- a/crypto/internal.h
> +++ b/crypto/internal.h
> @@ -68,13 +68,28 @@ void crypto_remove_final(struct list_head *list);
>  void crypto_shoot_alg(struct crypto_alg *alg);
>  struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
>  				      u32 mask);
> -void *crypto_create_tfm(struct crypto_alg *alg,
> -			const struct crypto_type *frontend);
> +void *crypto_create_tfm_node(struct crypto_alg *alg,
> +			const struct crypto_type *frontend, int node);
> +
> +static inline void *crypto_create_tfm(struct crypto_alg *alg,
> +			const struct crypto_type *frontend)
> +{
> +	return crypto_create_tfm_node(alg, frontend, NUMA_NO_NODE);
> +}
> +
>  struct crypto_alg *crypto_find_alg(const char *alg_name,
>  				   const struct crypto_type *frontend,
>  				   u32 type, u32 mask);
> -void *crypto_alloc_tfm(const char *alg_name,
> -		       const struct crypto_type *frontend, u32 type, u32 mask);
> +
> +void *crypto_alloc_tfm_node(const char *alg_name,
> +		       const struct crypto_type *frontend, u32 type, u32 mask,
> +		       int node);
> +
> +static inline void *crypto_alloc_tfm(const char *alg_name,
> +		       const struct crypto_type *frontend, u32 type, u32 mask)
> +{
> +	return crypto_alloc_tfm_node(alg_name, frontend, type, mask, NUMA_NO_NODE);
> +}
>  
>  int crypto_probing_notify(unsigned long val, void *v);
>  
> diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
> index 2b4d2b06ccbd..b1a78687014a 100644
> --- a/include/crypto/acompress.h
> +++ b/include/crypto/acompress.h
> @@ -106,6 +106,13 @@ struct acomp_alg {
>   */
>  struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32 type,
>  					u32 mask);
> +/**
> + * crypto_alloc_acomp_node() -- allocate ACOMPRESS tfm handle with desired NUMA
> + *				node

Given slightly relaxed view on 80 chars now in place, I'd put that on one line.

Also kernel-doc needs to be complete so though it's tedious you should document
the other parameters.

> + * @node:	specifies the NUMA node the ZIP hardware belongs to
> + */
> +struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name, u32 type,
> +					u32 mask, int node);
>  
>  static inline struct crypto_tfm *crypto_acomp_tfm(struct crypto_acomp *tfm)
>  {
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 763863dbc079..c1a47ce4c09e 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -593,8 +593,9 @@ int crypto_has_alg(const char *name, u32 type, u32 mask);
>   */
>  
>  struct crypto_tfm {
> -

Stray change. Shouldn't be in this patch.


>  	u32 crt_flags;
> +
> +	int node;
>  	
>  	void (*exit)(struct crypto_tfm *tfm);
>  	

RE: [PATCH 1/3] crypto: permit users to specify numa node of acomp hardware

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron
> Sent: Monday, June 22, 2020 9:59 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: herbert@gondor.apana.org.au; davem@davemloft.net; Seth Jennings
> <sjenning@redhat.com>; Linuxarm <linuxarm@huawei.com>;
> linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> linux-crypto@vger.kernel.org; akpm@linux-foundation.org; Dan Streetman
> <ddstreet@ieee.org>; Vitaly Wool <vitaly.wool@konsulko.com>
> Subject: Re: [PATCH 1/3] crypto: permit users to specify numa node of acomp
> hardware
> 
> On Mon, 22 Jun 2020 14:48:59 +1200
> Barry Song <song.bao.hua@hisilicon.com> wrote:
> 
> > For a Linux server with NUMA, there are possibly multiple (de)compressors
> > which are either local or remote to some NUMA node. Some drivers will
> > automatically use the (de)compressor near the CPU calling acomp_alloc().
> > However, it is not necessarily correct because users who send acomp_req
> > could be from different NUMA node with the CPU which allocates acomp.
> >
> > Just like kernel has kmalloc() and kmalloc_node(), here crypto can have
> > same support.
> >
> > Cc: Seth Jennings <sjenning@redhat.com>
> > Cc: Dan Streetman <ddstreet@ieee.org>
> > Cc: Vitaly Wool <vitaly.wool@konsulko.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> 
> Hi Barry,
> 
> Seems sensible to me.  A few trivial comments inline.

Thanks for your review, Jonathan.

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  crypto/acompress.c         |  8 ++++++++
> >  crypto/api.c               | 22 ++++++++++++++--------
> >  crypto/internal.h          | 23 +++++++++++++++++++----
> >  include/crypto/acompress.h |  7 +++++++
> >  include/linux/crypto.h     |  3 ++-
> >  5 files changed, 50 insertions(+), 13 deletions(-)
> >
> > diff --git a/crypto/acompress.c b/crypto/acompress.c
> > index 84a76723e851..c32c72048a1c 100644
> > --- a/crypto/acompress.c
> > +++ b/crypto/acompress.c
> > @@ -109,6 +109,14 @@ struct crypto_acomp *crypto_alloc_acomp(const
> char *alg_name, u32 type,
> >  }
> >  EXPORT_SYMBOL_GPL(crypto_alloc_acomp);
> >
> > +struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name,
> u32 type,
> > +					u32 mask, int node)
> > +{
> > +	return crypto_alloc_tfm_node(alg_name, &crypto_acomp_type, type,
> mask,
> > +				node);
> > +}
> > +EXPORT_SYMBOL_GPL(crypto_alloc_acomp_node);
> > +
> >  struct acomp_req *acomp_request_alloc(struct crypto_acomp *acomp)
> >  {
> >  	struct crypto_tfm *tfm = crypto_acomp_tfm(acomp);
> > diff --git a/crypto/api.c b/crypto/api.c
> > index edcf690800d4..4ecf712286af 100644
> > --- a/crypto/api.c
> > +++ b/crypto/api.c
> > @@ -433,8 +433,9 @@ struct crypto_tfm *crypto_alloc_base(const char
> *alg_name, u32 type, u32 mask)
> >  }
> >  EXPORT_SYMBOL_GPL(crypto_alloc_base);
> >
> > -void *crypto_create_tfm(struct crypto_alg *alg,
> > -			const struct crypto_type *frontend)
> > +void *crypto_create_tfm_node(struct crypto_alg *alg,
> > +			const struct crypto_type *frontend,
> > +			int node)
> >  {
> >  	char *mem;
> >  	struct crypto_tfm *tfm = NULL;
> > @@ -451,6 +452,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
> >
> >  	tfm = (struct crypto_tfm *)(mem + tfmsize);
> >  	tfm->__crt_alg = alg;
> > +	tfm->node = node;
> >
> >  	err = frontend->init_tfm(tfm);
> >  	if (err)
> > @@ -472,7 +474,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
> >  out:
> >  	return mem;
> >  }
> > -EXPORT_SYMBOL_GPL(crypto_create_tfm);
> > +EXPORT_SYMBOL_GPL(crypto_create_tfm_node);
> >
> >  struct crypto_alg *crypto_find_alg(const char *alg_name,
> >  				   const struct crypto_type *frontend,
> > @@ -490,11 +492,13 @@ struct crypto_alg *crypto_find_alg(const char
> *alg_name,
> >  EXPORT_SYMBOL_GPL(crypto_find_alg);
> >
> >  /*
> > - *	crypto_alloc_tfm - Locate algorithm and allocate transform
> > + *	crypto_alloc_tfm_node - Locate algorithm and allocate transform
> >   *	@alg_name: Name of algorithm
> >   *	@frontend: Frontend algorithm type
> >   *	@type: Type of algorithm
> >   *	@mask: Mask for type comparison
> > + *	@node: NUMA node in which users desire to put requests, if node is
> > + *		NUMA_NO_NODE, it means users have no special requirement.
> >   *
> >   *	crypto_alloc_tfm() will first attempt to locate an already loaded
> >   *	algorithm.  If that fails and the kernel supports dynamically
> loadable
> > @@ -509,8 +513,10 @@ EXPORT_SYMBOL_GPL(crypto_find_alg);
> >   *
> >   *	In case of error the return value is an error pointer.
> >   */
> > -void *crypto_alloc_tfm(const char *alg_name,
> > -		       const struct crypto_type *frontend, u32 type, u32 mask)
> > +
> > +void *crypto_alloc_tfm_node(const char *alg_name,
> > +		       const struct crypto_type *frontend, u32 type, u32 mask,
> > +		       int node)
> >  {
> >  	void *tfm;
> >  	int err;
> > @@ -524,7 +530,7 @@ void *crypto_alloc_tfm(const char *alg_name,
> >  			goto err;
> >  		}
> >
> > -		tfm = crypto_create_tfm(alg, frontend);
> > +		tfm = crypto_create_tfm_node(alg, frontend, node);
> >  		if (!IS_ERR(tfm))
> >  			return tfm;
> >
> > @@ -542,7 +548,7 @@ void *crypto_alloc_tfm(const char *alg_name,
> >
> >  	return ERR_PTR(err);
> >  }
> > -EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
> > +EXPORT_SYMBOL_GPL(crypto_alloc_tfm_node);
> >
> >  /*
> >   *	crypto_destroy_tfm - Free crypto transform
> > diff --git a/crypto/internal.h b/crypto/internal.h
> > index ff06a3bd1ca1..1b92a5a61852 100644
> > --- a/crypto/internal.h
> > +++ b/crypto/internal.h
> > @@ -68,13 +68,28 @@ void crypto_remove_final(struct list_head *list);
> >  void crypto_shoot_alg(struct crypto_alg *alg);
> >  struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
> >  				      u32 mask);
> > -void *crypto_create_tfm(struct crypto_alg *alg,
> > -			const struct crypto_type *frontend);
> > +void *crypto_create_tfm_node(struct crypto_alg *alg,
> > +			const struct crypto_type *frontend, int node);
> > +
> > +static inline void *crypto_create_tfm(struct crypto_alg *alg,
> > +			const struct crypto_type *frontend)
> > +{
> > +	return crypto_create_tfm_node(alg, frontend, NUMA_NO_NODE);
> > +}
> > +
> >  struct crypto_alg *crypto_find_alg(const char *alg_name,
> >  				   const struct crypto_type *frontend,
> >  				   u32 type, u32 mask);
> > -void *crypto_alloc_tfm(const char *alg_name,
> > -		       const struct crypto_type *frontend, u32 type, u32 mask);
> > +
> > +void *crypto_alloc_tfm_node(const char *alg_name,
> > +		       const struct crypto_type *frontend, u32 type, u32 mask,
> > +		       int node);
> > +
> > +static inline void *crypto_alloc_tfm(const char *alg_name,
> > +		       const struct crypto_type *frontend, u32 type, u32 mask)
> > +{
> > +	return crypto_alloc_tfm_node(alg_name, frontend, type, mask,
> NUMA_NO_NODE);
> > +}
> >
> >  int crypto_probing_notify(unsigned long val, void *v);
> >
> > diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
> > index 2b4d2b06ccbd..b1a78687014a 100644
> > --- a/include/crypto/acompress.h
> > +++ b/include/crypto/acompress.h
> > @@ -106,6 +106,13 @@ struct acomp_alg {
> >   */
> >  struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32
> type,
> >  					u32 mask);
> > +/**
> > + * crypto_alloc_acomp_node() -- allocate ACOMPRESS tfm handle with
> desired NUMA
> > + *				node
> 
> Given slightly relaxed view on 80 chars now in place, I'd put that on one line.
> 

Will put in another line.

> Also kernel-doc needs to be complete so though it's tedious you should
> document
> the other parameters.

You mean here the doc for other parameters should be copied from crypto_alloc_acomp()? 
If so, it is ok to me :-)

> 
> > + * @node:	specifies the NUMA node the ZIP hardware belongs to
> > + */
> > +struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name,
> u32 type,
> > +					u32 mask, int node);
> >
> >  static inline struct crypto_tfm *crypto_acomp_tfm(struct crypto_acomp
> *tfm)
> >  {
> > diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> > index 763863dbc079..c1a47ce4c09e 100644
> > --- a/include/linux/crypto.h
> > +++ b/include/linux/crypto.h
> > @@ -593,8 +593,9 @@ int crypto_has_alg(const char *name, u32 type, u32
> mask);
> >   */
> >
> >  struct crypto_tfm {
> > -
> 
> Stray change. Shouldn't be in this patch.

Will have a separate patch for this.

> 
> 
> >  	u32 crt_flags;
> > +
> > +	int node;
> >
> >  	void (*exit)(struct crypto_tfm *tfm);
> >
> 

Best Regard
Barry

Re: [PATCH 1/3] crypto: permit users to specify numa node of acomp hardware

[Jonathan Cameron]

On Mon, 22 Jun 2020 11:24:23 +0100
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron
> > Sent: Monday, June 22, 2020 9:59 PM
> > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> > Cc: herbert@gondor.apana.org.au; davem@davemloft.net; Seth Jennings
> > <sjenning@redhat.com>; Linuxarm <linuxarm@huawei.com>;
> > linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> > linux-crypto@vger.kernel.org; akpm@linux-foundation.org; Dan Streetman
> > <ddstreet@ieee.org>; Vitaly Wool <vitaly.wool@konsulko.com>
> > Subject: Re: [PATCH 1/3] crypto: permit users to specify numa node of acomp
> > hardware
> > 
> > On Mon, 22 Jun 2020 14:48:59 +1200
> > Barry Song <song.bao.hua@hisilicon.com> wrote:
> >   
> > > For a Linux server with NUMA, there are possibly multiple (de)compressors
> > > which are either local or remote to some NUMA node. Some drivers will
> > > automatically use the (de)compressor near the CPU calling acomp_alloc().
> > > However, it is not necessarily correct because users who send acomp_req
> > > could be from different NUMA node with the CPU which allocates acomp.
> > >
> > > Just like kernel has kmalloc() and kmalloc_node(), here crypto can have
> > > same support.
> > >
> > > Cc: Seth Jennings <sjenning@redhat.com>
> > > Cc: Dan Streetman <ddstreet@ieee.org>
> > > Cc: Vitaly Wool <vitaly.wool@konsulko.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>  
> > 
> > Hi Barry,
> > 
> > Seems sensible to me.  A few trivial comments inline.  
> 
> Thanks for your review, Jonathan.
> 
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > >  crypto/acompress.c         |  8 ++++++++
> > >  crypto/api.c               | 22 ++++++++++++++--------
> > >  crypto/internal.h          | 23 +++++++++++++++++++----
> > >  include/crypto/acompress.h |  7 +++++++
> > >  include/linux/crypto.h     |  3 ++-
> > >  5 files changed, 50 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/crypto/acompress.c b/crypto/acompress.c
> > > index 84a76723e851..c32c72048a1c 100644
> > > --- a/crypto/acompress.c
> > > +++ b/crypto/acompress.c
> > > @@ -109,6 +109,14 @@ struct crypto_acomp *crypto_alloc_acomp(const  
> > char *alg_name, u32 type,  
> > >  }
> > >  EXPORT_SYMBOL_GPL(crypto_alloc_acomp);
> > >
> > > +struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name,  
> > u32 type,  
> > > +					u32 mask, int node)
> > > +{
> > > +	return crypto_alloc_tfm_node(alg_name, &crypto_acomp_type, type,  
> > mask,  
> > > +				node);
> > > +}
> > > +EXPORT_SYMBOL_GPL(crypto_alloc_acomp_node);
> > > +
> > >  struct acomp_req *acomp_request_alloc(struct crypto_acomp *acomp)
> > >  {
> > >  	struct crypto_tfm *tfm = crypto_acomp_tfm(acomp);
> > > diff --git a/crypto/api.c b/crypto/api.c
> > > index edcf690800d4..4ecf712286af 100644
> > > --- a/crypto/api.c
> > > +++ b/crypto/api.c
> > > @@ -433,8 +433,9 @@ struct crypto_tfm *crypto_alloc_base(const char  
> > *alg_name, u32 type, u32 mask)  
> > >  }
> > >  EXPORT_SYMBOL_GPL(crypto_alloc_base);
> > >
> > > -void *crypto_create_tfm(struct crypto_alg *alg,
> > > -			const struct crypto_type *frontend)
> > > +void *crypto_create_tfm_node(struct crypto_alg *alg,
> > > +			const struct crypto_type *frontend,
> > > +			int node)
> > >  {
> > >  	char *mem;
> > >  	struct crypto_tfm *tfm = NULL;
> > > @@ -451,6 +452,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
> > >
> > >  	tfm = (struct crypto_tfm *)(mem + tfmsize);
> > >  	tfm->__crt_alg = alg;
> > > +	tfm->node = node;
> > >
> > >  	err = frontend->init_tfm(tfm);
> > >  	if (err)
> > > @@ -472,7 +474,7 @@ void *crypto_create_tfm(struct crypto_alg *alg,
> > >  out:
> > >  	return mem;
> > >  }
> > > -EXPORT_SYMBOL_GPL(crypto_create_tfm);
> > > +EXPORT_SYMBOL_GPL(crypto_create_tfm_node);
> > >
> > >  struct crypto_alg *crypto_find_alg(const char *alg_name,
> > >  				   const struct crypto_type *frontend,
> > > @@ -490,11 +492,13 @@ struct crypto_alg *crypto_find_alg(const char  
> > *alg_name,  
> > >  EXPORT_SYMBOL_GPL(crypto_find_alg);
> > >
> > >  /*
> > > - *	crypto_alloc_tfm - Locate algorithm and allocate transform
> > > + *	crypto_alloc_tfm_node - Locate algorithm and allocate transform
> > >   *	@alg_name: Name of algorithm
> > >   *	@frontend: Frontend algorithm type
> > >   *	@type: Type of algorithm
> > >   *	@mask: Mask for type comparison
> > > + *	@node: NUMA node in which users desire to put requests, if node is
> > > + *		NUMA_NO_NODE, it means users have no special requirement.
> > >   *
> > >   *	crypto_alloc_tfm() will first attempt to locate an already loaded
> > >   *	algorithm.  If that fails and the kernel supports dynamically  
> > loadable  
> > > @@ -509,8 +513,10 @@ EXPORT_SYMBOL_GPL(crypto_find_alg);
> > >   *
> > >   *	In case of error the return value is an error pointer.
> > >   */
> > > -void *crypto_alloc_tfm(const char *alg_name,
> > > -		       const struct crypto_type *frontend, u32 type, u32 mask)
> > > +
> > > +void *crypto_alloc_tfm_node(const char *alg_name,
> > > +		       const struct crypto_type *frontend, u32 type, u32 mask,
> > > +		       int node)
> > >  {
> > >  	void *tfm;
> > >  	int err;
> > > @@ -524,7 +530,7 @@ void *crypto_alloc_tfm(const char *alg_name,
> > >  			goto err;
> > >  		}
> > >
> > > -		tfm = crypto_create_tfm(alg, frontend);
> > > +		tfm = crypto_create_tfm_node(alg, frontend, node);
> > >  		if (!IS_ERR(tfm))
> > >  			return tfm;
> > >
> > > @@ -542,7 +548,7 @@ void *crypto_alloc_tfm(const char *alg_name,
> > >
> > >  	return ERR_PTR(err);
> > >  }
> > > -EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
> > > +EXPORT_SYMBOL_GPL(crypto_alloc_tfm_node);
> > >
> > >  /*
> > >   *	crypto_destroy_tfm - Free crypto transform
> > > diff --git a/crypto/internal.h b/crypto/internal.h
> > > index ff06a3bd1ca1..1b92a5a61852 100644
> > > --- a/crypto/internal.h
> > > +++ b/crypto/internal.h
> > > @@ -68,13 +68,28 @@ void crypto_remove_final(struct list_head *list);
> > >  void crypto_shoot_alg(struct crypto_alg *alg);
> > >  struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
> > >  				      u32 mask);
> > > -void *crypto_create_tfm(struct crypto_alg *alg,
> > > -			const struct crypto_type *frontend);
> > > +void *crypto_create_tfm_node(struct crypto_alg *alg,
> > > +			const struct crypto_type *frontend, int node);
> > > +
> > > +static inline void *crypto_create_tfm(struct crypto_alg *alg,
> > > +			const struct crypto_type *frontend)
> > > +{
> > > +	return crypto_create_tfm_node(alg, frontend, NUMA_NO_NODE);
> > > +}
> > > +
> > >  struct crypto_alg *crypto_find_alg(const char *alg_name,
> > >  				   const struct crypto_type *frontend,
> > >  				   u32 type, u32 mask);
> > > -void *crypto_alloc_tfm(const char *alg_name,
> > > -		       const struct crypto_type *frontend, u32 type, u32 mask);
> > > +
> > > +void *crypto_alloc_tfm_node(const char *alg_name,
> > > +		       const struct crypto_type *frontend, u32 type, u32 mask,
> > > +		       int node);
> > > +
> > > +static inline void *crypto_alloc_tfm(const char *alg_name,
> > > +		       const struct crypto_type *frontend, u32 type, u32 mask)
> > > +{
> > > +	return crypto_alloc_tfm_node(alg_name, frontend, type, mask,  
> > NUMA_NO_NODE);  
> > > +}
> > >
> > >  int crypto_probing_notify(unsigned long val, void *v);
> > >
> > > diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
> > > index 2b4d2b06ccbd..b1a78687014a 100644
> > > --- a/include/crypto/acompress.h
> > > +++ b/include/crypto/acompress.h
> > > @@ -106,6 +106,13 @@ struct acomp_alg {
> > >   */
> > >  struct crypto_acomp *crypto_alloc_acomp(const char *alg_name, u32  
> > type,  
> > >  					u32 mask);
> > > +/**
> > > + * crypto_alloc_acomp_node() -- allocate ACOMPRESS tfm handle with  
> > desired NUMA  
> > > + *				node  
> > 
> > Given slightly relaxed view on 80 chars now in place, I'd put that on one line.
> >   
> 
> Will put in another line.
> 
> > Also kernel-doc needs to be complete so though it's tedious you should
> > document
> > the other parameters.  
> 
> You mean here the doc for other parameters should be copied from crypto_alloc_acomp()? 
> If so, it is ok to me :-)

Yes.  Always worth sanity checking new kernel-doc by
running the script over the file and fixing any warnings you have introduced.

> 
> >   
> > > + * @node:	specifies the NUMA node the ZIP hardware belongs to
> > > + */
> > > +struct crypto_acomp *crypto_alloc_acomp_node(const char *alg_name,  
> > u32 type,  
> > > +					u32 mask, int node);
> > >
> > >  static inline struct crypto_tfm *crypto_acomp_tfm(struct crypto_acomp  
> > *tfm)  
> > >  {
> > > diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> > > index 763863dbc079..c1a47ce4c09e 100644
> > > --- a/include/linux/crypto.h
> > > +++ b/include/linux/crypto.h
> > > @@ -593,8 +593,9 @@ int crypto_has_alg(const char *name, u32 type, u32  
> > mask);  
> > >   */
> > >
> > >  struct crypto_tfm {
> > > -  
> > 
> > Stray change. Shouldn't be in this patch.  
> 
> Will have a separate patch for this.

Is it really worth bothering?  I'd just leave a blank line there. Feels harmless!

> 
> > 
> >   
> > >  	u32 crt_flags;
> > > +
> > > +	int node;
> > >
> > >  	void (*exit)(struct crypto_tfm *tfm);
> > >  
> >   
> 
> Best Regard
> Barry
>