Date:   Wed, 6 Feb 2019 17:30:27 +0000
From:   Jonathan Cameron <jonathan.cameron@...wei.com>
To:     Keith Busch <keith.busch@...el.com>
CC:     <linux-kernel@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
        <linux-mm@...ck.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael Wysocki" <rafael@...nel.org>,
        Dave Hansen <dave.hansen@...el.com>,
        "Dan Williams" <dan.j.williams@...el.com>, <linuxarm@...wei.com>
Subject: Re: [PATCHv5 00/10] Heterogeneuos memory node attributes

On Wed, 6 Feb 2019 10:19:37 -0700
Keith Busch <keith.busch@...el.com> wrote:

> On Wed, Feb 06, 2019 at 12:31:00PM +0000, Jonathan Cameron wrote:
> > On Thu, 24 Jan 2019 16:07:14 -0700
> > Keith Busch <keith.busch@...el.com> wrote:
> > 
> > 1) It seems this version added a hard dependence on having the memory node
> >    listed in the Memory Proximity Domain attribute structures.  I'm not 100%
> >    sure there is actually any requirement to have those structures. If you aren't
> >    using the hint bit, they don't convey any information.  It could be argued
> >    that they provide info on what is found in the other hmat entries, but there
> >    is little purpose as those entries are explicit in what the provide.
> >    (Given I didn't have any of these structures and things  worked fine with
> >     v4 it seems this is a new check).  
> 
> Right, v4 just used the node(s) with the highest performance. You mentioned
> systems having nodes with different performance, but no winner across all
> attributes, so there's no clear way to rank these for access class linkage.
> Requiring an initiator PXM present clears that up.
> 
> Maybe we can fallback to performance if the initiator pxm isn't provided,
> but the ranking is going to require an arbitrary decision, like prioritize
> latency over bandwidth.

I'd certainly prefer to see that fall back and would argue it is
the only valid route.  What is 'best' if we don't put a preference on
one parameter over the other.

Perfectly fine to have another access class that does bandwidth preferred
if that is of sufficient use to people.

>  
> >    This is also somewhat inconsistent.
> >    a) If a given entry isn't there, we still get for example
> >       node4/access0/initiators/[read|write]_* but all values are 0.
> >       If we want to do the check you have it needs to not create the files in
> >       this case.  Whilst they have no meaning as there are no initiators, it
> >       is inconsistent to my mind.
> > 
> >    b) Having one "Memory Proximity Domain attribute structure" for node 4 linking
> >       it to node0 is sufficient to allow
> >       node4/access0/initiators/node0
> >       node4/access0/initiators/node1
> >       node4/access0/initiators/node2
> >       node4/access0/initiators/node3
> >       I think if we are going to enforce the presence of that structure then only
> >       the node0 link should exist.  
> 
> We'd link the initiator pxm in the Address Range Structure, and also any
> other nodes with identical performance access. I think that makes sense.

I disagree on this. It is either / or, it seem really illogical to build
all of them if only one initiator is specified for the target.

If someone deliberately only specified one initiator for this target then they
meant to do that (hopefully).  Probably because they wanted to set one
of the flags.

>  
> > 2) Error handling could perhaps do to spit out some nasty warnings.
> >    If we have an entry for nodes that don't exist we shouldn't just fail silently,
> >    that's just one example I managed to trigger with minor table tweaking.
> > 
> > Personally I would just get rid of enforcing anything based on the presence of
> > that structure.  

Thanks,

Jonathan

Powered by blists - more mailing lists