* [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 related [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
* [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 related [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 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).