Messages in this thread | | | Date | Tue, 6 Dec 2011 12:45:28 -0800 (PST) | From | David Rientjes <> | Subject | Re: NUMA x86: add constraints check for nid parameters |
| |
On Fri, 2 Dec 2011, Petr Holasek wrote:
> > > > > This patch adds constraints checks into __node_distance() and > > > > > numa_set_distance() functions. If from or to parameters are > > > > > lower than zero, it results into oops now. > > > > > > > > Passing negative numbers into __node_distance() sounds like a bug in > > > > the caller, and this patch will remove our means of detecting that bug. > > > > > > That's true, but upper boundary is checked now, so why not to check lower? > > > > Because it adds more code to the kernel and can hide bugs? > >
The upper bound is checked to ensure that we don't dereference past end of the array that stores the distance table, so it will catch errors for things like memory hotplug when additional nodes are onlined and the data structure isn't updated accordingly.
> > If what we're doing here is to be defensive against buggy BIOS tables > > (a good idea) then we should validate the BIOS table values as close as > > possible to the point where they were read frmo the BIOS. And we should > > (probably) emit a warning if a bad table entry is detected, rather than > > silently fixing it up. > > numa_set_distance() does exactly what you described above, only emits a > warning. I agree with your objections with __node_distance() checks, it > really can hide bugs in caller. So silent fix-up is the main problem and > we shouldn't check anything so the caller will be advised when using > wrong nid by oops with a benefit of less code for us. Do I understand your > opinion on this type of code?
I'd have no objection to adding a check to numa_set_distance() to ensure the node ids are non-negative in the same way we check that the distances themselves are non-negative; that can catch errors when pxms are used uninitialized when parsing the SRAT. However, I think adding the check to __node_distance() is unnecessary.
| |