Re: [PATCH v4 5/5] nvdimm: Schedule device registration on node local to the device

[Alexander Duyck <alexander.h.duyck@linux.intel.com>]

On 9/20/2018 7:46 PM, Dan Williams wrote:
> On Thu, Sep 20, 2018 at 6:34 PM Alexander Duyck
> <alexander.h.duyck@linux.intel.com> wrote:
>>
>>
>>
>> On 9/20/2018 5:36 PM, Dan Williams wrote:
>>> On Thu, Sep 20, 2018 at 5:26 PM Alexander Duyck
>>> <alexander.h.duyck@linux.intel.com> wrote:
>>>>
>>>> On 9/20/2018 3:59 PM, Dan Williams wrote:
>>>>> On Thu, Sep 20, 2018 at 3:31 PM Alexander Duyck
>>>>> <alexander.h.duyck@linux.intel.com> wrote:
>>>>>>
>>>>>> This patch is meant to force the device registration for nvdimm devices to
>>>>>> be closer to the actual device. This is achieved by using either the NUMA
>>>>>> node ID of the region, or of the parent. By doing this we can have
>>>>>> everything above the region based on the region, and everything below the
>>>>>> region based on the nvdimm bus.
>>>>>>
>>>>>> One additional change I made is that we hold onto a reference to the parent
>>>>>> while we are going through registration. By doing this we can guarantee we
>>>>>> can complete the registration before we have the parent device removed.
>>>>>>
>>>>>> By guaranteeing NUMA locality I see an improvement of as high as 25% for
>>>>>> per-node init of a system with 12TB of persistent memory.
>>>>>>
>>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> ---
>>>>>>     drivers/nvdimm/bus.c |   19 +++++++++++++++++--
>>>>>>     1 file changed, 17 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>>>>>> index 8aae6dcc839f..ca935296d55e 100644
>>>>>> --- a/drivers/nvdimm/bus.c
>>>>>> +++ b/drivers/nvdimm/bus.c
>>>>>> @@ -487,7 +487,9 @@ static void nd_async_device_register(void *d, async_cookie_t cookie)
>>>>>>                    dev_err(dev, "%s: failed\n", __func__);
>>>>>>                    put_device(dev);
>>>>>>            }
>>>>>> +
>>>>>>            put_device(dev);
>>>>>> +       put_device(dev->parent);
>>>>>
>>>>> Good catch. The child does not pin the parent until registration, but
>>>>> we need to make sure the parent isn't gone while were waiting for the
>>>>> registration work to run.
>>>>>
>>>>> Let's break this reference count fix out into its own separate patch,
>>>>> because this looks to be covering a gap that may need to be
>>>>> recommended for -stable.
>>>>
>>>> Okay, I guess I can do that.
>>>>
>>>>>
>>>>>>
>>>>>>     static void nd_async_device_unregister(void *d, async_cookie_t cookie)
>>>>>> @@ -504,12 +506,25 @@ static void nd_async_device_unregister(void *d, async_cookie_t cookie)
>>>>>>
>>>>>>     void __nd_device_register(struct device *dev)
>>>>>>     {
>>>>>> +       int node;
>>>>>> +
>>>>>>            if (!dev)
>>>>>>                    return;
>>>>>> +
>>>>>>            dev->bus = &nvdimm_bus_type;
>>>>>> +       get_device(dev->parent);
>>>>>>            get_device(dev);
>>>>>> -       async_schedule_domain(nd_async_device_register, dev,
>>>>>> -                       &nd_async_domain);
>>>>>> +
>>>>>> +       /*
>>>>>> +        * For a region we can break away from the parent node,
>>>>>> +        * otherwise for all other devices we just inherit the node from
>>>>>> +        * the parent.
>>>>>> +        */
>>>>>> +       node = is_nd_region(dev) ? to_nd_region(dev)->numa_node :
>>>>>> +                                  dev_to_node(dev->parent);
>>>>>
>>>>> Devices already automatically inherit the node of their parent, so I'm
>>>>> not understanding why this is needed?
>>>>
>>>> That doesn't happen until you call device_add, which you don't call
>>>> until nd_async_device_register. All that has been called on the device
>>>> up to now is device_initialize which leaves the node at NUMA_NO_NODE.
>>>
>>> Ooh, yeah, missed that. I think I'd prefer this policy to moved out to
>>> where we set the dev->parent before calling __nd_device_register, or
>>> at least a comment here about *why* we know region devices are special
>>> (i.e. because the nd_region_desc specified the node at region creation
>>> time).
>>>
>>
>> Are you talking about pulling the scheduling out or just adding a node
>> value to the nd_device_register call so it can be set directly from the
>> caller?
> 
> I was thinking everywhere we set dev->parent before registering, also
> set the node...

That will not work unless we move the call to device_initialize to 
somewhere before you are setting the node. That is why I was thinking it 
might work to put the node assignment in nd_device_register itself since 
it looks like the regions don't call __nd_device_register directly.

I guess we could get rid of nd_device_register if we wanted to go that 
route.

>> If you wanted what I could do is pull the set_dev_node call from
>> nvdimm_bus_uevent and place it in nd_device_register. That should stick
>> as the node doesn't get overwritten by the parent if it is set after
>> device_initialize. If I did that along with the parent bit I was already
>> doing then all that would be left to do in is just use the dev_to_node
>> call on the device itself.
> 
> ...but this is even better.
> 

I'm not sure it adds that much. Basically My thought was we just need to 
make sure to set the device node after the call to device_initialize but 
before the call to device_add. This just seems like a bunch more work 
spread the device_initialize calls all over and introduce possible 
regressions.