Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-11 Thread Tadeusz Struk
On 10/10/2014 03:15 PM, Prarit Bhargava wrote: In short, that calcuation is wrong. Don't use it; stick with the widely accepted and used dev_to_node of the pci_dev. It is used in other cases IIRC to determine the numa location of the device. It shouldn't be any different for this driver.

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-10 Thread Prarit Bhargava
On 10/09/2014 07:12 PM, Tadeusz Struk wrote: On 10/09/2014 02:42 PM, Prarit Bhargava wrote: I don't think cpu hotplug matters here. This is one (probe) time determination if the configuration is optimal or not and if it makes sense to use this accelerator or not. It absolutely matters.

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-10 Thread Tadeusz Struk
On 10/10/2014 04:23 AM, Prarit Bhargava wrote: Sure, but I still think that we are safe here. No, you're not. Dropping a single CPU changes num_online_cpus(), which results in static uint8_t adf_get_dev_node_id(struct pci_dev *pdev) { unsigned int bus_per_cpu = 0;

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-10 Thread Prarit Bhargava
On 10/10/2014 09:25 AM, Tadeusz Struk wrote: On 10/10/2014 04:23 AM, Prarit Bhargava wrote: Sure, but I still think that we are safe here. No, you're not. Dropping a single CPU changes num_online_cpus(), which results in static uint8_t adf_get_dev_node_id(struct pci_dev *pdev) {

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-09 Thread Prarit Bhargava
[sorry ... accidentally hit reply instead of reply all ... resending to everyone] On 10/08/2014 03:25 PM, Tadeusz Struk wrote: 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:

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-09 Thread Tadeusz Struk
On 10/09/2014 04:23 AM, Prarit Bhargava wrote: int numa_node; /* NUMA node this device is close to */ ... That's just bad english. The numa node value (for pci devices) is read from the ACPI tables on the system and represents the node that the pci_dev is connected to. }; In case

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-09 Thread Prarit Bhargava
On 10/09/2014 12:14 PM, Tadeusz Struk wrote: On 10/09/2014 04:23 AM, Prarit Bhargava wrote: int numa_node; /* NUMA node this device is close to */ ... That's just bad english. The numa node value (for pci devices) is read from the ACPI tables on the system and represents the node that

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-09 Thread Tadeusz Struk
On 10/09/2014 10:32 AM, Prarit Bhargava wrote: This calculation is sole for multi-socket configuration. This is why is was introduced and what it was tested for. There is no point discussing NUMA for single-socket configuration. Single socket configurations are not NUMA. In this case

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-09 Thread Prarit Bhargava
On 10/09/2014 03:55 PM, Tadeusz Struk wrote: On 10/09/2014 10:32 AM, Prarit Bhargava wrote: This calculation is sole for multi-socket configuration. This is why is was introduced and what it was tested for. There is no point discussing NUMA for single-socket configuration. Single socket

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-09 Thread Tadeusz Struk
On 10/09/2014 02:42 PM, Prarit Bhargava wrote: I don't think cpu hotplug matters here. This is one (probe) time determination if the configuration is optimal or not and if it makes sense to use this accelerator or not. It absolutely matters. num_online_cpus() *changes* depending on the #

[PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
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

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Prarit Bhargava
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

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
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

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Prarit Bhargava
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

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
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

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Prarit Bhargava
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

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
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