nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Minor fixups for nd_device_register
@ 2018-09-25 20:52 Alexander Duyck
  2018-09-25 20:53 ` [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init Alexander Duyck
  2018-09-25 20:53 ` [PATCH 2/2] nvdimm: Set device node in nd_device_register Alexander Duyck
  0 siblings, 2 replies; 9+ messages in thread
From: Alexander Duyck @ 2018-09-25 20:52 UTC (permalink / raw)
  To: linux-kernel, linux-nvdimm; +Cc: zwisler

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init
  2018-09-25 20:52 [PATCH 0/2] Minor fixups for nd_device_register Alexander Duyck
@ 2018-09-25 20:53 ` Alexander Duyck
  2018-09-25 21:09   ` Dan Williams
  2018-09-25 20:53 ` [PATCH 2/2] nvdimm: Set device node in nd_device_register Alexander Duyck
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2018-09-25 20:53 UTC (permalink / raw)
  To: linux-kernel, linux-nvdimm; +Cc: zwisler

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] nvdimm: Set device node in nd_device_register
  2018-09-25 20:52 [PATCH 0/2] Minor fixups for nd_device_register Alexander Duyck
  2018-09-25 20:53 ` [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init Alexander Duyck
@ 2018-09-25 20:53 ` Alexander Duyck
  2018-09-25 21:08   ` Dan Williams
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2018-09-25 20:53 UTC (permalink / raw)
  To: linux-kernel, linux-nvdimm; +Cc: zwisler

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register
  2018-09-25 20:53 ` [PATCH 2/2] nvdimm: Set device node in nd_device_register Alexander Duyck
@ 2018-09-25 21:08   ` Dan Williams
  2018-09-25 21:11     ` Alexander Duyck
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Williams @ 2018-09-25 21:08 UTC (permalink / raw)
  To: alexander.h.duyck; +Cc: zwisler, Linux Kernel Mailing List, linux-nvdimm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init
  2018-09-25 20:53 ` [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init Alexander Duyck
@ 2018-09-25 21:09   ` Dan Williams
  0 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-09-25 21:09 UTC (permalink / raw)
  To: alexander.h.duyck; +Cc: zwisler, Linux Kernel Mailing List, linux-nvdimm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register
  2018-09-25 21:08   ` Dan Williams
@ 2018-09-25 21:11     ` Alexander Duyck
  2018-09-25 21:32       ` Dan Williams
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2018-09-25 21:11 UTC (permalink / raw)
  To: Dan Williams; +Cc: zwisler, Linux Kernel Mailing List, linux-nvdimm



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register
  2018-09-25 21:11     ` Alexander Duyck
@ 2018-09-25 21:32       ` Dan Williams
  2018-09-25 21:35         ` Alexander Duyck
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Williams @ 2018-09-25 21:32 UTC (permalink / raw)
  To: alexander.h.duyck; +Cc: zwisler, Linux Kernel Mailing List, linux-nvdimm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register
  2018-09-25 21:32       ` Dan Williams
@ 2018-09-25 21:35         ` Alexander Duyck
  2018-09-25 21:50           ` Williams, Dan J
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2018-09-25 21:35 UTC (permalink / raw)
  To: Dan Williams; +Cc: zwisler, Linux Kernel Mailing List, linux-nvdimm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] nvdimm: Set device node in nd_device_register
  2018-09-25 21:35         ` Alexander Duyck
@ 2018-09-25 21:50           ` Williams, Dan J
  0 siblings, 0 replies; 9+ messages in thread
From: Williams, Dan J @ 2018-09-25 21:50 UTC (permalink / raw)
  To: alexander.h.duyck; +Cc: zwisler, linux-kernel, linux-nvdimm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-09-25 21:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-25 20:52 [PATCH 0/2] Minor fixups for nd_device_register Alexander Duyck
2018-09-25 20:53 ` [PATCH 1/2] nvdimm: Hold reference on parent while scheduling async init Alexander Duyck
2018-09-25 21:09   ` Dan Williams
2018-09-25 20:53 ` [PATCH 2/2] nvdimm: Set device node in nd_device_register Alexander Duyck
2018-09-25 21:08   ` Dan Williams
2018-09-25 21:11     ` Alexander Duyck
2018-09-25 21:32       ` Dan Williams
2018-09-25 21:35         ` Alexander Duyck
2018-09-25 21:50           ` Williams, Dan J

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).