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

[Prarit Bhargava <prarit@redhat.com>]

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)
>> {
>>         unsigned int bus_per_cpu = 0;
>>         struct cpuinfo_x86 *c = &cpu_data(num_online_cpus() - 1);  <<< this
>> being different.
>>
>>         if (!c->phys_proc_id)
>>                 return 0;
>>
>>         bus_per_cpu = 256 / (c->phys_proc_id + 1);  <<< this being different
>>
>>         if (bus_per_cpu != 0)
>>                 return pdev->bus->number / bus_per_cpu; <<< and this being different
>>         return 0;
>> }
> 
> You forgot to explain how this is not safe.

Sorry, I thought I did explain it.  My apologies.

So let's say you boot the system and load the driver.  At this time,

num_online_cpus@boot = 4 .  Crunch through the math above, and you reference the
cpuinfo_x86 struct for cpu 3 (the "fourth" cpu), and the calculation takes into
account c->phys_proc_id.

So let's say now you boot the system and disable a cpu.  In this case, now
num_online_cpus@module_load = 3.  Crunch through the math above and you're
referncing a different cpuinfo_x86 struct for cpu 2.  That may or may not point
at the same c->phys_proc_id.  That changes the calculation and gives an
incorrect value.

In addition to that I haven't even talked about the possibility of hot-adding
and hot-removing cpus in sockets which changes the numbering scheme completely.

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.

P.
> T.
>