[PATCH 0/2] Minor fixups for nd_device_register

[PATCH 0/2] Minor fixups for nd_device_register

[Alexander Duyck]

This patch series addresses a couple minor issues in nd_device_register.

The first patch resolves an issue in which we weren't holding the parent
lock so it is possible the parent could have been released prior to us
taking care of initializing the device via the asynchronous init.

The second patch moves the initialization of the device node to the
earliest point where we can do it. By doing this we can avoid repeatedly
updating the node on each uevent. 

---

Alexander Duyck (2):
      nvdimm: Hold reference on parent while scheduling async init
      nvdimm: Set device node in nd_device_register


 drivers/nvdimm/bus.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

--
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init

[Alexander Duyck]

Unlike asynchronous initialization in the core we have not yet associated
the device with the parent, and as such the device doesn't hold a reference
to the parent.

In order to resolve that we should be holding a reference on the parent
until the asynchronous initialization has completed.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/nvdimm/bus.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 8aae6dcc839f..9148015ed803 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -488,6 +488,8 @@ static void nd_async_device_register(void *d, async_cookie_t cookie)
 		put_device(dev);
 	}
 	put_device(dev);
+	if (dev->parent)
+		put_device(dev->parent);
 }
 
 static void nd_async_device_unregister(void *d, async_cookie_t cookie)
@@ -507,6 +509,8 @@ void __nd_device_register(struct device *dev)
 	if (!dev)
 		return;
 	dev->bus = &nvdimm_bus_type;
+	if (dev->parent)
+		get_device(dev->parent);
 	get_device(dev);
 	async_schedule_domain(nd_async_device_register, dev,
 			&nd_async_domain);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/2] nvdimm: Set device node in nd_device_register

[Alexander Duyck]

This change makes it so that we don't repeatedly overwrite the device node
for nvdimm regions. The earliest we can set the node is immediately after
calling device init, so I have moved the code there so we can avoid
rewriting the node with each uevent.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/nvdimm/bus.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 9148015ed803..96f4d0e1706a 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -54,12 +54,6 @@ static int to_nd_device_type(struct device *dev)
 
 static int nvdimm_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	/*
-	 * Ensure that region devices always have their numa node set as
-	 * early as possible.
-	 */
-	if (is_nd_region(dev))
-		set_dev_node(dev, to_nd_region(dev)->numa_node);
 	return add_uevent_var(env, "MODALIAS=" ND_DEVICE_MODALIAS_FMT,
 			to_nd_device_type(dev));
 }
@@ -519,6 +513,16 @@ void __nd_device_register(struct device *dev)
 void nd_device_register(struct device *dev)
 {
 	device_initialize(dev);
+
+	/*
+	 * Ensure that region devices always have their NUMA node set as
+	 * early as possible. This way we are able to make certain that any
+	 * memory associated with the creation and the creation itself of
+	 * the region is associated with the correct node.
+	 */
+	if (is_nd_region(dev))
+		set_dev_node(dev, to_nd_region(dev)->numa_node);
+
 	__nd_device_register(dev);
 }
 EXPORT_SYMBOL(nd_device_register);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register

[Dan Williams]

On Tue, Sep 25, 2018 at 1:56 PM Alexander Duyck
<alexander.h.duyck@linux.intel.com> wrote:
>
> This change makes it so that we don't repeatedly overwrite the device node
> for nvdimm regions. The earliest we can set the node is immediately after
> calling device init, so I have moved the code there so we can avoid
> rewriting the node with each uevent.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/nvdimm/bus.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 9148015ed803..96f4d0e1706a 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -54,12 +54,6 @@ static int to_nd_device_type(struct device *dev)
>
>  static int nvdimm_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> -       /*
> -        * Ensure that region devices always have their numa node set as
> -        * early as possible.
> -        */
> -       if (is_nd_region(dev))
> -               set_dev_node(dev, to_nd_region(dev)->numa_node);
>         return add_uevent_var(env, "MODALIAS=" ND_DEVICE_MODALIAS_FMT,
>                         to_nd_device_type(dev));
>  }
> @@ -519,6 +513,16 @@ void __nd_device_register(struct device *dev)
>  void nd_device_register(struct device *dev)
>  {
>         device_initialize(dev);
> +
> +       /*
> +        * Ensure that region devices always have their NUMA node set as
> +        * early as possible. This way we are able to make certain that any
> +        * memory associated with the creation and the creation itself of
> +        * the region is associated with the correct node.
> +        */
> +       if (is_nd_region(dev))
> +               set_dev_node(dev, to_nd_region(dev)->numa_node);
> +
>         __nd_device_register(dev);

Any reason to not put this inside __nd_device_register()? If you're ok
with that I can just fix up when applying.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init

[Dan Williams]

On Tue, Sep 25, 2018 at 1:53 PM Alexander Duyck
<alexander.h.duyck@linux.intel.com> wrote:
>
> Unlike asynchronous initialization in the core we have not yet associated
> the device with the parent, and as such the device doesn't hold a reference
> to the parent.
>
> In order to resolve that we should be holding a reference on the parent
> until the asynchronous initialization has completed.

Looks good to me. Thanks for splitting this out, applied.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register

[Alexander Duyck]

On 9/25/2018 2:08 PM, Dan Williams wrote:
> On Tue, Sep 25, 2018 at 1:56 PM Alexander Duyck
> <alexander.h.duyck@linux.intel.com> wrote:
>>
>> This change makes it so that we don't repeatedly overwrite the device node
>> for nvdimm regions. The earliest we can set the node is immediately after
>> calling device init, so I have moved the code there so we can avoid
>> rewriting the node with each uevent.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>> ---
>>   drivers/nvdimm/bus.c |   16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>> index 9148015ed803..96f4d0e1706a 100644
>> --- a/drivers/nvdimm/bus.c
>> +++ b/drivers/nvdimm/bus.c
>> @@ -54,12 +54,6 @@ static int to_nd_device_type(struct device *dev)
>>
>>   static int nvdimm_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>>   {
>> -       /*
>> -        * Ensure that region devices always have their numa node set as
>> -        * early as possible.
>> -        */
>> -       if (is_nd_region(dev))
>> -               set_dev_node(dev, to_nd_region(dev)->numa_node);
>>          return add_uevent_var(env, "MODALIAS=" ND_DEVICE_MODALIAS_FMT,
>>                          to_nd_device_type(dev));
>>   }
>> @@ -519,6 +513,16 @@ void __nd_device_register(struct device *dev)
>>   void nd_device_register(struct device *dev)
>>   {
>>          device_initialize(dev);
>> +
>> +       /*
>> +        * Ensure that region devices always have their NUMA node set as
>> +        * early as possible. This way we are able to make certain that any
>> +        * memory associated with the creation and the creation itself of
>> +        * the region is associated with the correct node.
>> +        */
>> +       if (is_nd_region(dev))
>> +               set_dev_node(dev, to_nd_region(dev)->numa_node);
>> +
>>          __nd_device_register(dev);
> 
> Any reason to not put this inside __nd_device_register()? If you're ok
> with that I can just fix up when applying.

I put it here since regions never call __nd_device_register directly, 
they always call nd_device_register. I figured that by placing it here 
we reduce the impact of this on the other devices that aren't going to 
need it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register

[Dan Williams]

On Tue, Sep 25, 2018 at 2:11 PM Alexander Duyck
<alexander.h.duyck@linux.intel.com> wrote:
>
>
>
> On 9/25/2018 2:08 PM, Dan Williams wrote:
> > On Tue, Sep 25, 2018 at 1:56 PM Alexander Duyck
> > <alexander.h.duyck@linux.intel.com> wrote:
> >>
> >> This change makes it so that we don't repeatedly overwrite the device node
> >> for nvdimm regions. The earliest we can set the node is immediately after
> >> calling device init, so I have moved the code there so we can avoid
> >> rewriting the node with each uevent.
> >>
> >> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >> ---
> >>   drivers/nvdimm/bus.c |   16 ++++++++++------
> >>   1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> >> index 9148015ed803..96f4d0e1706a 100644
> >> --- a/drivers/nvdimm/bus.c
> >> +++ b/drivers/nvdimm/bus.c
> >> @@ -54,12 +54,6 @@ static int to_nd_device_type(struct device *dev)
> >>
> >>   static int nvdimm_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >>   {
> >> -       /*
> >> -        * Ensure that region devices always have their numa node set as
> >> -        * early as possible.
> >> -        */
> >> -       if (is_nd_region(dev))
> >> -               set_dev_node(dev, to_nd_region(dev)->numa_node);
> >>          return add_uevent_var(env, "MODALIAS=" ND_DEVICE_MODALIAS_FMT,
> >>                          to_nd_device_type(dev));
> >>   }
> >> @@ -519,6 +513,16 @@ void __nd_device_register(struct device *dev)
> >>   void nd_device_register(struct device *dev)
> >>   {
> >>          device_initialize(dev);
> >> +
> >> +       /*
> >> +        * Ensure that region devices always have their NUMA node set as
> >> +        * early as possible. This way we are able to make certain that any
> >> +        * memory associated with the creation and the creation itself of
> >> +        * the region is associated with the correct node.
> >> +        */
> >> +       if (is_nd_region(dev))
> >> +               set_dev_node(dev, to_nd_region(dev)->numa_node);
> >> +
> >>          __nd_device_register(dev);
> >
> > Any reason to not put this inside __nd_device_register()? If you're ok
> > with that I can just fix up when applying.
>
> I put it here since regions never call __nd_device_register directly,
> they always call nd_device_register. I figured that by placing it here
> we reduce the impact of this on the other devices that aren't going to
> need it.

I can't imagine there's much of an impact because we're already
reading the cacheline that has dev->parent so dev->type is already
there. If we're going to add support for looking up the parent node to
schedule namespace registration work to the proper core I would expect
that to appear in the __nd_device_register() common code as well.

In general the only consideration for calling nd_device_register() vs
__nd_device_register() is whether or not you want a
device_initialize() to occur.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register

[Alexander Duyck]

On 9/25/2018 2:32 PM, Dan Williams wrote:
> On Tue, Sep 25, 2018 at 2:11 PM Alexander Duyck
> <alexander.h.duyck@linux.intel.com> wrote:
>>
>>
>>
>> On 9/25/2018 2:08 PM, Dan Williams wrote:
>>> On Tue, Sep 25, 2018 at 1:56 PM Alexander Duyck
>>> <alexander.h.duyck@linux.intel.com> wrote:
>>>>
>>>> This change makes it so that we don't repeatedly overwrite the device node
>>>> for nvdimm regions. The earliest we can set the node is immediately after
>>>> calling device init, so I have moved the code there so we can avoid
>>>> rewriting the node with each uevent.
>>>>
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> ---
>>>>    drivers/nvdimm/bus.c |   16 ++++++++++------
>>>>    1 file changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>>>> index 9148015ed803..96f4d0e1706a 100644
>>>> --- a/drivers/nvdimm/bus.c
>>>> +++ b/drivers/nvdimm/bus.c
>>>> @@ -54,12 +54,6 @@ static int to_nd_device_type(struct device *dev)
>>>>
>>>>    static int nvdimm_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>>>>    {
>>>> -       /*
>>>> -        * Ensure that region devices always have their numa node set as
>>>> -        * early as possible.
>>>> -        */
>>>> -       if (is_nd_region(dev))
>>>> -               set_dev_node(dev, to_nd_region(dev)->numa_node);
>>>>           return add_uevent_var(env, "MODALIAS=" ND_DEVICE_MODALIAS_FMT,
>>>>                           to_nd_device_type(dev));
>>>>    }
>>>> @@ -519,6 +513,16 @@ void __nd_device_register(struct device *dev)
>>>>    void nd_device_register(struct device *dev)
>>>>    {
>>>>           device_initialize(dev);
>>>> +
>>>> +       /*
>>>> +        * Ensure that region devices always have their NUMA node set as
>>>> +        * early as possible. This way we are able to make certain that any
>>>> +        * memory associated with the creation and the creation itself of
>>>> +        * the region is associated with the correct node.
>>>> +        */
>>>> +       if (is_nd_region(dev))
>>>> +               set_dev_node(dev, to_nd_region(dev)->numa_node);
>>>> +
>>>>           __nd_device_register(dev);
>>>
>>> Any reason to not put this inside __nd_device_register()? If you're ok
>>> with that I can just fix up when applying.
>>
>> I put it here since regions never call __nd_device_register directly,
>> they always call nd_device_register. I figured that by placing it here
>> we reduce the impact of this on the other devices that aren't going to
>> need it.
> 
> I can't imagine there's much of an impact because we're already
> reading the cacheline that has dev->parent so dev->type is already
> there. If we're going to add support for looking up the parent node to
> schedule namespace registration work to the proper core I would expect
> that to appear in the __nd_device_register() common code as well.
> 
> In general the only consideration for calling nd_device_register() vs
> __nd_device_register() is whether or not you want a
> device_initialize() to occur.

Yeah, I kind of gathered that. If you want to move it feel free to.

- Alex
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register

[Williams, Dan J]

On Tue, 2018-09-25 at 14:35 -0700, Alexander Duyck wrote:
[..]
> Yeah, I kind of gathered that. If you want to move it feel free to.

Thanks for the flexibility, applied.

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm