Re: [Xen-devel] [RFC PATCH v3 15/24] ARM: NUMA: DT: Add CPU NUMA support

[Julien Grall <julien.grall@xxxxxxx>]

On 25/07/17 19:48, Stefano Stabellini wrote:
> On Tue, 25 Jul 2017, Julien Grall wrote:
> > On 25/07/17 07:47, Vijay Kilari wrote:
> > > > >  void numa_failed(void)
> > > > >  {
> > > > >      numa_off = true;
> > > > >      init_dt_numa_distance();
> > > > >      node_distance_fn = NULL;
> > > > > +    init_cpu_to_node();
> > > > > +}
> > > > > +
> > > > > +void __init numa_set_cpu_node(int cpu, unsigned int nid)
> > > > > +{
> > > > > +    if ( !node_isset(nid, processor_nodes_parsed) || nid >=
> > > > > MAX_NUMNODES
> > > > > )
> > > > > +        nid = 0;
> > > >
> > > > This looks wrong to me. If the node-id is invalid, why would you blindly
> > > > set
> > > > to 0?
> > > Generally this check will not pass. I will make this function return
> > > error code in case
> > > of wrong nid.
> > I don't really want to see error code and error handling everywhere in the
> > initialization code. I would assume that if the NUMA bindings are wrong we
> > should just crash Xen rather continuing with NUMA disabled.
> >
> > Stefano do you have any opinion here?
> Yes, I noticed that there is an overabundance of error checks in the
> patches. I have pointed out in other cases that some of these checks are
> duplicates.
>
> I am OK with some checks but we should not do the same check over and
> over.
>
> To answer the question: do we need any checks at all?
>
> I am fine with no checks on the device tree or ACPI bindings themselves.
> I am also OK with some checks in few places to check that the
> information passed by the firmware is in the right shape (for example we
> check for the ACPI header length before accessing any ACPI tables). That
> is good. But I am not OK with repeating the same check multiple times
> uselessly or checking for conditions that cannot happen (like a NULL
> pointer in the ACPI header case again).
I would prefer to keep the check on the DT bindings and ACPI bindings. I 
hit some problem in the past that were quite annoying to debug without them.
But I was wondering if we should just panic/BUG_ON directly. Rather than 
returning an error.
For any redundant check, we could just turn to an ASSERT.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel