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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 25 Jul 2017, Julien Grall wrote:
> 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.

I think BUG_ON is fine, but it would be best if we also printed a
useful message before crashing Xen. At least the user would know that
the problem is a broken device_tree/ACPI.


> For any redundant check, we could just turn to an ASSERT.

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