[2/2] nvdimm: Set device node in nd_device_register
diff mbox series

Message ID 20180925205307.6182.26453.stgit@localhost.localdomain
State New, archived
Headers show
Series
  • Minor fixups for nd_device_register
Related show

Commit Message

Alexander Duyck Sept. 25, 2018, 8:53 p.m. UTC
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(-)

Comments

Dan Williams Sept. 25, 2018, 9:08 p.m. UTC | #1
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.
Alexander Duyck Sept. 25, 2018, 9:11 p.m. UTC | #2
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.
Dan Williams Sept. 25, 2018, 9:32 p.m. UTC | #3
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.
Alexander Duyck Sept. 25, 2018, 9:35 p.m. UTC | #4
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
Dan Williams Sept. 25, 2018, 9:50 p.m. UTC | #5
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.

Patch
diff mbox series

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);