Re: memset() in crypto code?
On 10/08/2014 04:30 AM, Sandy Harris wrote: I have started a thread about this on the gcc help mailing list https://gcc.gnu.org/ml/gcc-help/2014-10/msg00047.html Great, perhaps you want to pass a patch proposal to gcc folks? We might consider replacinging memzero_explicit with memset_s() since that is in the C!! standard, albeit I think as optional. IBM, Apple, NetBSD, ... have that. https://mail-index.netbsd.org/tech-userlevel/2012/02/24/msg006125.html The patch you point to for NetBSD does nothing else what memzero_explicit() or bzero_explicit() variants already internally do, only that they're discussing about whether to adopt it or not in their user space libc ... Cheers, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto, qat, use generic numa functions
On 10/08/2014 11:50 AM, Tadeusz Struk wrote: Hi Prarit, On 10/07/2014 05:12 PM, Prarit Bhargava wrote: The method in which the qat code determines the numa node for memory allocations is a bit clunky. On 2 socket, single node systems it is possible that adf_get_dev_node_id() returns node 1, even though node 1 doesn't exist. This code transitions the qat code to the generic numa functions. Changing adf_get_dev_node_id() to a simple call to dev_get_node() results in a change to the adf_accel_dev struct as well. The problem with that is we don't want to use any valid numa node, but the node we are connected to or we don't want to use the accelerator at all. Otherwise, when the first valid numa node happens to be the remote node the dma transactions we be slow and instead of accelerating we will slow things down. A patch that enforces this is on it's way. Yeah, I was actually wondering if dev_get_node() returns NO_NODE, then we should just default to 0? I'll wait for your patch ... P. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto, qat, use generic numa functions
Hi Prarit, On 10/07/2014 05:12 PM, Prarit Bhargava wrote: The method in which the qat code determines the numa node for memory allocations is a bit clunky. On 2 socket, single node systems it is possible that adf_get_dev_node_id() returns node 1, even though node 1 doesn't exist. This code transitions the qat code to the generic numa functions. Changing adf_get_dev_node_id() to a simple call to dev_get_node() results in a change to the adf_accel_dev struct as well. The problem with that is we don't want to use any valid numa node, but the node we are connected to or we don't want to use the accelerator at all. Otherwise, when the first valid numa node happens to be the remote node the dma transactions we be slow and instead of accelerating we will slow things down. A patch that enforces this is on it's way. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] crypto: qat - Fix for invalid dma mapping and numa
Hi, These two patches fix invalid (zero length) dma mapping and enforce numa configuration for maximum performance. Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com --- Tadeusz Struk (2): crypto: qat - Prevent dma mapping zero length assoc data. crypto: qat - Enforce valid numa configuration. drivers/crypto/qat/qat_common/adf_accel_devices.h |3 +-- drivers/crypto/qat/qat_common/adf_transport.c | 12 +++- drivers/crypto/qat/qat_common/qat_algs.c |7 +-- drivers/crypto/qat/qat_common/qat_crypto.c|8 +--- drivers/crypto/qat/qat_dh895xcc/adf_admin.c |2 +- drivers/crypto/qat/qat_dh895xcc/adf_drv.c |9 - drivers/crypto/qat/qat_dh895xcc/adf_isr.c |2 +- 7 files changed, 28 insertions(+), 15 deletions(-) -- -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] crypto: qat - Enforce valid numa configuration.
In a system with NUMA configuration we want to enforce that the accelerator is connected to a node with memory to avoid cross QPI memory transaction. Otherwise there is no point in using the accelerator as the encryption in software will be faster. Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com --- drivers/crypto/qat/qat_common/adf_accel_devices.h |3 +-- drivers/crypto/qat/qat_common/adf_transport.c | 12 +++- drivers/crypto/qat/qat_common/qat_algs.c |5 +++-- drivers/crypto/qat/qat_common/qat_crypto.c|8 +--- drivers/crypto/qat/qat_dh895xcc/adf_admin.c |2 +- drivers/crypto/qat/qat_dh895xcc/adf_drv.c |9 - drivers/crypto/qat/qat_dh895xcc/adf_isr.c |2 +- 7 files changed, 26 insertions(+), 15 deletions(-) diff --git a/drivers/crypto/qat/qat_common/adf_accel_devices.h b/drivers/crypto/qat/qat_common/adf_accel_devices.h index 3cfe195..96e0b06 100644 --- a/drivers/crypto/qat/qat_common/adf_accel_devices.h +++ b/drivers/crypto/qat/qat_common/adf_accel_devices.h @@ -203,8 +203,7 @@ struct adf_accel_dev { struct dentry *debugfs_dir; struct list_head list; struct module *owner; - uint8_t accel_id; - uint8_t numa_node; struct adf_accel_pci accel_pci_dev; + uint8_t accel_id; } __packed; #endif diff --git a/drivers/crypto/qat/qat_common/adf_transport.c b/drivers/crypto/qat/qat_common/adf_transport.c index 5f3fa45..9dd2cb7 100644 --- a/drivers/crypto/qat/qat_common/adf_transport.c +++ b/drivers/crypto/qat/qat_common/adf_transport.c @@ -419,9 +419,10 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev, WRITE_CSR_RING_BASE(csr_addr, bank_num, i, 0); ring = bank-rings[i]; if (hw_data-tx_rings_mask (1 i)) { - ring-inflights = kzalloc_node(sizeof(atomic_t), - GFP_KERNEL, - accel_dev-numa_node); + ring-inflights = + kzalloc_node(sizeof(atomic_t), +GFP_KERNEL, +dev_to_node(GET_DEV(accel_dev))); if (!ring-inflights) goto err; } else { @@ -469,13 +470,14 @@ int adf_init_etr_data(struct adf_accel_dev *accel_dev) int i, ret; etr_data = kzalloc_node(sizeof(*etr_data), GFP_KERNEL, - accel_dev-numa_node); + dev_to_node(GET_DEV(accel_dev))); if (!etr_data) return -ENOMEM; num_banks = GET_MAX_BANKS(accel_dev); size = num_banks * sizeof(struct adf_etr_bank_data); - etr_data-banks = kzalloc_node(size, GFP_KERNEL, accel_dev-numa_node); + etr_data-banks = kzalloc_node(size, GFP_KERNEL, + dev_to_node(GET_DEV(accel_dev))); if (!etr_data-banks) { ret = -ENOMEM; goto err_bank; diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c index bffa8bf..0897a1c 100644 --- a/drivers/crypto/qat/qat_common/qat_algs.c +++ b/drivers/crypto/qat/qat_common/qat_algs.c @@ -598,7 +598,8 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance *inst, if (unlikely(!n)) return -EINVAL; - bufl = kmalloc_node(sz, GFP_ATOMIC, inst-accel_dev-numa_node); + bufl = kmalloc_node(sz, GFP_ATOMIC, + dev_to_node(GET_DEV(inst-accel_dev))); if (unlikely(!bufl)) return -ENOMEM; @@ -644,7 +645,7 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance *inst, struct qat_alg_buf *bufers; buflout = kmalloc_node(sz, GFP_ATOMIC, - inst-accel_dev-numa_node); + dev_to_node(GET_DEV(inst-accel_dev))); if (unlikely(!buflout)) goto err; bloutp = dma_map_single(dev, buflout, sz, DMA_TO_DEVICE); diff --git a/drivers/crypto/qat/qat_common/qat_crypto.c b/drivers/crypto/qat/qat_common/qat_crypto.c index 060dc0a..c1eefc4 100644 --- a/drivers/crypto/qat/qat_common/qat_crypto.c +++ b/drivers/crypto/qat/qat_common/qat_crypto.c @@ -109,12 +109,14 @@ struct qat_crypto_instance *qat_crypto_get_instance_node(int node) list_for_each(itr, adf_devmgr_get_head()) { accel_dev = list_entry(itr, struct adf_accel_dev, list); - if (accel_dev-numa_node == node adf_dev_started(accel_dev)) + if ((node == dev_to_node(GET_DEV(accel_dev)) || + dev_to_node(GET_DEV(accel_dev)) 0) +adf_dev_started(accel_dev)) break;
[PATCH 1/2] crypto: qat - Prevent dma mapping zero length assoc data.
Do not attempt to dma map associated data if it is zero length. Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com --- drivers/crypto/qat/qat_common/qat_algs.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c index e03afe9..bffa8bf 100644 --- a/drivers/crypto/qat/qat_common/qat_algs.c +++ b/drivers/crypto/qat/qat_common/qat_algs.c @@ -607,6 +607,8 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance *inst, goto err; for_each_sg(assoc, sg, assoc_n, i) { + if (!sg-length) + continue; bufl-bufers[bufs].addr = dma_map_single(dev, sg_virt(sg), sg-length, -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/08/2014 01:38 PM, Tadeusz Struk wrote: In a system with NUMA configuration we want to enforce that the accelerator is connected to a node with memory to avoid cross QPI memory transaction. Otherwise there is no point in using the accelerator as the encryption in software will be faster. Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com --- drivers/crypto/qat/qat_common/adf_accel_devices.h |3 +-- drivers/crypto/qat/qat_common/adf_transport.c | 12 +++- drivers/crypto/qat/qat_common/qat_algs.c |5 +++-- drivers/crypto/qat/qat_common/qat_crypto.c|8 +--- drivers/crypto/qat/qat_dh895xcc/adf_admin.c |2 +- drivers/crypto/qat/qat_dh895xcc/adf_drv.c |9 - drivers/crypto/qat/qat_dh895xcc/adf_isr.c |2 +- 7 files changed, 26 insertions(+), 15 deletions(-) diff --git a/drivers/crypto/qat/qat_common/adf_accel_devices.h b/drivers/crypto/qat/qat_common/adf_accel_devices.h index 3cfe195..96e0b06 100644 --- a/drivers/crypto/qat/qat_common/adf_accel_devices.h +++ b/drivers/crypto/qat/qat_common/adf_accel_devices.h @@ -203,8 +203,7 @@ struct adf_accel_dev { struct dentry *debugfs_dir; struct list_head list; struct module *owner; - uint8_t accel_id; - uint8_t numa_node; struct adf_accel_pci accel_pci_dev; + uint8_t accel_id; } __packed; #endif diff --git a/drivers/crypto/qat/qat_common/adf_transport.c b/drivers/crypto/qat/qat_common/adf_transport.c index 5f3fa45..9dd2cb7 100644 --- a/drivers/crypto/qat/qat_common/adf_transport.c +++ b/drivers/crypto/qat/qat_common/adf_transport.c @@ -419,9 +419,10 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev, WRITE_CSR_RING_BASE(csr_addr, bank_num, i, 0); ring = bank-rings[i]; if (hw_data-tx_rings_mask (1 i)) { - ring-inflights = kzalloc_node(sizeof(atomic_t), -GFP_KERNEL, -accel_dev-numa_node); + ring-inflights = + kzalloc_node(sizeof(atomic_t), + GFP_KERNEL, + dev_to_node(GET_DEV(accel_dev))); if (!ring-inflights) goto err; } else { @@ -469,13 +470,14 @@ int adf_init_etr_data(struct adf_accel_dev *accel_dev) int i, ret; etr_data = kzalloc_node(sizeof(*etr_data), GFP_KERNEL, - accel_dev-numa_node); + dev_to_node(GET_DEV(accel_dev))); if (!etr_data) return -ENOMEM; num_banks = GET_MAX_BANKS(accel_dev); size = num_banks * sizeof(struct adf_etr_bank_data); - etr_data-banks = kzalloc_node(size, GFP_KERNEL, accel_dev-numa_node); + etr_data-banks = kzalloc_node(size, GFP_KERNEL, +dev_to_node(GET_DEV(accel_dev))); if (!etr_data-banks) { ret = -ENOMEM; goto err_bank; diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c index bffa8bf..0897a1c 100644 --- a/drivers/crypto/qat/qat_common/qat_algs.c +++ b/drivers/crypto/qat/qat_common/qat_algs.c @@ -598,7 +598,8 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance *inst, if (unlikely(!n)) return -EINVAL; - bufl = kmalloc_node(sz, GFP_ATOMIC, inst-accel_dev-numa_node); + bufl = kmalloc_node(sz, GFP_ATOMIC, + dev_to_node(GET_DEV(inst-accel_dev))); if (unlikely(!bufl)) return -ENOMEM; @@ -644,7 +645,7 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance *inst, struct qat_alg_buf *bufers; buflout = kmalloc_node(sz, GFP_ATOMIC, -inst-accel_dev-numa_node); +dev_to_node(GET_DEV(inst-accel_dev))); if (unlikely(!buflout)) goto err; bloutp = dma_map_single(dev, buflout, sz, DMA_TO_DEVICE); diff --git a/drivers/crypto/qat/qat_common/qat_crypto.c b/drivers/crypto/qat/qat_common/qat_crypto.c index 060dc0a..c1eefc4 100644 --- a/drivers/crypto/qat/qat_common/qat_crypto.c +++ b/drivers/crypto/qat/qat_common/qat_crypto.c @@ -109,12 +109,14 @@ struct qat_crypto_instance *qat_crypto_get_instance_node(int node) list_for_each(itr, adf_devmgr_get_head()) { accel_dev = list_entry(itr, struct adf_accel_dev, list); - if (accel_dev-numa_node == node adf_dev_started(accel_dev)) + if ((node == dev_to_node(GET_DEV(accel_dev)) || + dev_to_node(GET_DEV(accel_dev)) 0) + adf_dev_started(accel_dev))
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/08/2014 10:57 AM, Prarit Bhargava wrote: node = adf_get_dev_node_id(pdev); ^^^ I don't think you should ever make this call. IMO it is wrong to do it that way. Just stick with node = dev_to_node(pdev-dev) as the line below forces a default to that anyway. But then how do I know which node I'm physically connected to? + if (node != dev_to_node(pdev-dev) dev_to_node(pdev-dev) 0) { + /* If the accelerator is connected to a node with no memory + * there is no point in using the accelerator since the remote + * memory transaction will be very slow. */ + dev_err(pdev-dev, Invalid NUMA configuration.\n); + return -EINVAL; Hmm ... I wonder if it would be safe to do /* force allocations to node 0 */ node = 0; dev_err(pdev-dev, Invalid NUMA configuration detected, node id = %d . Defaulting node to 0. \n, node); and then continue? As the comment say there is no point continuing if the configuration is wrong. Defaulting to 0 will cause the same panic you pointed out in your first patch if node 0 has no memory. And maybe even a FW_WARN of some sort here might be appropriate to indicate that something is wrong with the mapping? In any case a better error message is a always good idea IMO. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/08/2014 02:11 PM, Tadeusz Struk wrote: On 10/08/2014 10:57 AM, Prarit Bhargava wrote: node = adf_get_dev_node_id(pdev); ^^^ I don't think you should ever make this call. IMO it is wrong to do it that way. Just stick with node = dev_to_node(pdev-dev) as the line below forces a default to that anyway. But then how do I know which node I'm physically connected to? The pci_dev maps to the bus which maps to a numa node. The pci_dev's numa value is copied directly from the bus (or busses depending on how deep it is). I'd argue (strongly) that the pci_dev's numa ID better be correct o/w that is a FW bug (and a bad one at that these days). dev_to_node() should return the correct value. + if (node != dev_to_node(pdev-dev) dev_to_node(pdev-dev) 0) { + /* If the accelerator is connected to a node with no memory + * there is no point in using the accelerator since the remote + * memory transaction will be very slow. */ + dev_err(pdev-dev, Invalid NUMA configuration.\n); + return -EINVAL; Hmm ... I wonder if it would be safe to do /* force allocations to node 0 */ node = 0; dev_err(pdev-dev, Invalid NUMA configuration detected, node id = %d . Defaulting node to 0. \n, node); and then continue? As the comment say there is no point continuing if the configuration is wrong. Defaulting to 0 will cause the same panic you pointed out in your first patch if node 0 has no memory. Okay, but at least fix up the warning message to output the node_id. That's sort of the important piece here. P. And maybe even a FW_WARN of some sort here might be appropriate to indicate that something is wrong with the mapping? In any case a better error message is a always good idea IMO. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/08/2014 11:35 AM, Prarit Bhargava wrote: But then how do I know which node I'm physically connected to? The pci_dev maps to the bus which maps to a numa node. The pci_dev's numa value is copied directly from the bus (or busses depending on how deep it is). I'd argue (strongly) that the pci_dev's numa ID better be correct o/w that is a FW bug (and a bad one at that these days). dev_to_node() should return the correct value. I'm not saying that the dev_to_node() returns incorrect value. It will always return the closest numa node for the given device. What we want to enforce is that the closest numa node is the node that the device is physically connected to. In case if the closest numa node is the remote node we don't want to use this accelerator. +if (node != dev_to_node(pdev-dev) dev_to_node(pdev-dev) 0) { +/* If the accelerator is connected to a node with no memory + * there is no point in using the accelerator since the remote + * memory transaction will be very slow. */ +dev_err(pdev-dev, Invalid NUMA configuration.\n); +return -EINVAL; Hmm ... I wonder if it would be safe to do /* force allocations to node 0 */ node = 0; dev_err(pdev-dev, Invalid NUMA configuration detected, node id = %d . Defaulting node to 0. \n, node); and then continue? As the comment say there is no point continuing if the configuration is wrong. Defaulting to 0 will cause the same panic you pointed out in your first patch if node 0 has no memory. Okay, but at least fix up the warning message to output the node_id. That's sort of the important piece here. Sure, I can update the message add print both the numa node and node id the device is connected to. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/08/2014 02:57 PM, Tadeusz Struk wrote: On 10/08/2014 11:35 AM, Prarit Bhargava wrote: But then how do I know which node I'm physically connected to? The pci_dev maps to the bus which maps to a numa node. The pci_dev's numa value is copied directly from the bus (or busses depending on how deep it is). I'd argue (strongly) that the pci_dev's numa ID better be correct o/w that is a FW bug (and a bad one at that these days). dev_to_node() should return the correct value. I'm not saying that the dev_to_node() returns incorrect value. It will always return the closest numa node for the given device. No that isn't correct. dev_to_node() will return the node the device is connected to. What we want to enforce is that the closest numa node is the node that the device is physically connected to. In case if the closest numa node is the remote node we don't want to use this accelerator. P. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/08/2014 12:01 PM, Prarit Bhargava wrote: No that isn't correct. dev_to_node() will return the node the device is connected to. include/linux/device.h: static inline int dev_to_node(struct device *dev) { return dev-numa_node; } struct device { . int numa_node; /* NUMA node this device is close to */ ... }; In case when there are two nodes and only node 0 has memory, dev-numa_node will be 0 even though the device will be connected to the pci root port of node 1. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html