| Message ID | eea7049c-5a27-1498-8e8b-674d69468fbb@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: Doug Ledford [mailto:dledford@redhat.com] > Sent: Tuesday, May 17, 2016 6:49 PM > To: Mark Bloch <markb@mellanox.com>; leon@kernel.org > Cc: linux-rdma@vger.kernel.org > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > resolution module into core > > On 05/17/2016 04:29 AM, Mark Bloch wrote: > > > > > >> -----Original Message----- > >> From: Doug Ledford [mailto:dledford@redhat.com] > >> Sent: Monday, May 16, 2016 8:43 PM > >> To: leon@kernel.org > >> Cc: Mark Bloch <markb@mellanox.com>; linux-rdma@vger.kernel.org > >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > >> resolution module into core > >> Can you build netlink in and then init the ib_addr module after the > >> netlink init is complete? Wouldn't that resolve the dependency ordering > >> issue without changing the module names? > > Sorry, but I don't understand what do you mean by this. > > If you would like to keep the current module structure (ib_core.ko and > ib_addr.ko) > > The only way to use ibnl from within addr.c is to move the ibnl initialization > to addr.c > > and build netlink.c as part of ib_addr.ko, but it seems kind of strange if > > ib_addr is responsible to initialize ibnl. > > Not according to what I was looking at. In the original patch, you > moved addr.c into ib_core and changed addr_init and addr_cleanup from > static to normal. You were then able to control whether the addr code > went before or after the netlink code by rearranging the order of init > sequences in device.c:ib_core_init(). It is entirely possible that you > could leave ib_addr as a module, change addr_init to rdma_addr_init and > EXPORT_SYMBOL it, and likewise with the cleanup function, and remove the > module_init(addr_init); and module_exit(addr_cleanup); from the addr.c > file. This would then allow you to control when ib_addr was inited by > adding a call to rdma_addr_init() in device.c:ib_core_init() just like > you had in your patch. It would solve the problem and give you complete > control while being transparent to user space. This is what I mean: > > commit ec5394412286e325b83b6c64d026f4d7bab40ccd > Author: Doug Ledford <dledford@redhat.com> > Date: Tue May 17 11:43:57 2016 -0400 > > IB/core: change module init ordering > > Change ib_addr to be init'ed by ib_core and not by the module load > process. This gives us precise controle of when the module is > initialized while still preserving the existing module names and load > ordering. This is to be backward compatible with existing, older > SysV init scripts. When those older systems are dead and gone, we > can revisit this and change the module names and load orders however > we want. > > Signed-off-by: Doug Ledford <dledford@redhat.com> > > diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c > index 337353d86cfa..fda808546e0e 100644 > --- a/drivers/infiniband/core/addr.c > +++ b/drivers/infiniband/core/addr.c > @@ -634,7 +634,17 @@ static struct notifier_block nb = { > .notifier_call = netevent_callback > }; > > -static int __init addr_init(void) > +/* > + * This is a little unorthodox. We export our init function so that the > + * ib_core module can call it at the right time after the other modules > + * we depend on have registered. We do this solely because there are > some > + * user space init scripts that expect ib_addr to be loaded prior to > ib_core > + * and due to recent changes, we need the ib netlink code brought up > before > + * we bring this code up and that code is now in ib_core. When the older > + * SysV init script systems that have these init scripts are dead and gone, > + * we can revisit this and possibly just include addr.c in ib_core. > + */ > +int rdma_addr_init(void) > { > addr_wq = create_singlethread_workqueue("ib_addr"); > if (!addr_wq) > @@ -644,13 +654,12 @@ static int __init addr_init(void) > rdma_addr_register_client(&self); > return 0; > } > +EXPORT_SYMBOL(rdma_addr_init); > > -static void __exit addr_cleanup(void) > +void rdma_addr_cleanup(void) > { > rdma_addr_unregister_client(&self); > unregister_netevent_notifier(&nb); > destroy_workqueue(addr_wq); > } > - > -module_init(addr_init); > -module_exit(addr_cleanup); > +EXPORT_SYMBOL(rdma_addr_cleanup); > diff --git a/drivers/infiniband/core/core_priv.h > b/drivers/infiniband/core/core_priv.h > index eab32215756b..e0a5187ea98c 100644 > --- a/drivers/infiniband/core/core_priv.h > +++ b/drivers/infiniband/core/core_priv.h > @@ -137,4 +137,7 @@ static inline bool rdma_is_upper_dev_rcu(struct > net_device *dev, > return _upper == upper; > } > > +int rdma_addr_init(void); > +void rdma_addr_cleanup(void); > + > #endif /* _CORE_PRIV_H */ > diff --git a/drivers/infiniband/core/device.c > b/drivers/infiniband/core/device.c > index 10979844026a..4fdf87a485cc 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -983,10 +983,18 @@ static int __init ib_core_init(void) > goto err_sysfs; > } > > + ret = rdma_addr_init(); > + if (ret) { > + pr_warn("Couldn't init IB address resolution module\n"); > + goto err_ibnl; > + } > + > ib_cache_setup(); > > return 0; > > +err_ibnl: > + ibnl_cleanup(); > err_sysfs: > class_unregister(&ib_class); > err_comp: > @@ -999,6 +1007,7 @@ err: > static void __exit ib_core_cleanup(void) > { > ib_cache_cleanup(); > + rdma_addr_cleanup(); > ibnl_cleanup(); > class_unregister(&ib_class); > destroy_workqueue(ib_comp_wq); > > > > -- > Doug Ledford <dledford@redhat.com> > GPG KeyID: 0E572FDD > I'm not sure it will solve the problem. it has been a while since I've looked into how Linux handles modules so I might be wrong, but: When we build a module that has to use a symbol from a different module The symbol is unresolved (in has no address in the .ko file) the moment we load the module, The kernel searches for the address of the symbol, maps the module into memory and writes the correct address. Now in our case, verbs.c is part of ib_core.ko, and it uses rdma_addr_find_l2_eth_by_grh (which is in addr.c, part of ib_addr.ko) So the moment we try to load ib_core we need to have ib_addr already in memory (because we need the address of rdma_addr_find_l2_eth_by_grh) modprobe knows about that (because it sees there is a dependency between ib_core and ib_addr) so it tries to load ib_addr.ko first. ib_addr uses ibnl, so it has an unresolved symbol, which means ib_core.ko needs to be loaded before it, this is why we have a cycle. After all the theoretical mambo jambo, I had to check it and this is what I got: Using your code + my ibnl patches, the module_install step fails: depmod: ERROR: Found 2 modules in dependency cycles! depmod: ERROR: Cycle detected: ib_core -> ib_addr -> ib_core Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2016 02:43 AM, Mark Bloch wrote: > > >> -----Original Message----- >> From: Doug Ledford [mailto:dledford@redhat.com] >> Sent: Tuesday, May 17, 2016 6:49 PM >> To: Mark Bloch <markb@mellanox.com>; leon@kernel.org >> Cc: linux-rdma@vger.kernel.org >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address >> resolution module into core >> >> On 05/17/2016 04:29 AM, Mark Bloch wrote: >>> >>> >>>> -----Original Message----- >>>> From: Doug Ledford [mailto:dledford@redhat.com] >>>> Sent: Monday, May 16, 2016 8:43 PM >>>> To: leon@kernel.org >>>> Cc: Mark Bloch <markb@mellanox.com>; linux-rdma@vger.kernel.org >>>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address >>>> resolution module into core >>>> Can you build netlink in and then init the ib_addr module after the >>>> netlink init is complete? Wouldn't that resolve the dependency ordering >>>> issue without changing the module names? >>> Sorry, but I don't understand what do you mean by this. >>> If you would like to keep the current module structure (ib_core.ko and >> ib_addr.ko) >>> The only way to use ibnl from within addr.c is to move the ibnl initialization >> to addr.c >>> and build netlink.c as part of ib_addr.ko, but it seems kind of strange if >>> ib_addr is responsible to initialize ibnl. >> >> Not according to what I was looking at. In the original patch, you >> moved addr.c into ib_core and changed addr_init and addr_cleanup from >> static to normal. You were then able to control whether the addr code >> went before or after the netlink code by rearranging the order of init >> sequences in device.c:ib_core_init(). It is entirely possible that you >> could leave ib_addr as a module, change addr_init to rdma_addr_init and >> EXPORT_SYMBOL it, and likewise with the cleanup function, and remove the >> module_init(addr_init); and module_exit(addr_cleanup); from the addr.c >> file. This would then allow you to control when ib_addr was inited by >> adding a call to rdma_addr_init() in device.c:ib_core_init() just like >> you had in your patch. It would solve the problem and give you complete >> control while being transparent to user space. This is what I mean: >> >> commit ec5394412286e325b83b6c64d026f4d7bab40ccd >> Author: Doug Ledford <dledford@redhat.com> >> Date: Tue May 17 11:43:57 2016 -0400 >> >> IB/core: change module init ordering >> >> Change ib_addr to be init'ed by ib_core and not by the module load >> process. This gives us precise controle of when the module is >> initialized while still preserving the existing module names and load >> ordering. This is to be backward compatible with existing, older >> SysV init scripts. When those older systems are dead and gone, we >> can revisit this and change the module names and load orders however >> we want. >> >> Signed-off-by: Doug Ledford <dledford@redhat.com> >> >> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c >> index 337353d86cfa..fda808546e0e 100644 >> --- a/drivers/infiniband/core/addr.c >> +++ b/drivers/infiniband/core/addr.c >> @@ -634,7 +634,17 @@ static struct notifier_block nb = { >> .notifier_call = netevent_callback >> }; >> >> -static int __init addr_init(void) >> +/* >> + * This is a little unorthodox. We export our init function so that the >> + * ib_core module can call it at the right time after the other modules >> + * we depend on have registered. We do this solely because there are >> some >> + * user space init scripts that expect ib_addr to be loaded prior to >> ib_core >> + * and due to recent changes, we need the ib netlink code brought up >> before >> + * we bring this code up and that code is now in ib_core. When the older >> + * SysV init script systems that have these init scripts are dead and gone, >> + * we can revisit this and possibly just include addr.c in ib_core. >> + */ >> +int rdma_addr_init(void) >> { >> addr_wq = create_singlethread_workqueue("ib_addr"); >> if (!addr_wq) >> @@ -644,13 +654,12 @@ static int __init addr_init(void) >> rdma_addr_register_client(&self); >> return 0; >> } >> +EXPORT_SYMBOL(rdma_addr_init); >> >> -static void __exit addr_cleanup(void) >> +void rdma_addr_cleanup(void) >> { >> rdma_addr_unregister_client(&self); >> unregister_netevent_notifier(&nb); >> destroy_workqueue(addr_wq); >> } >> - >> -module_init(addr_init); >> -module_exit(addr_cleanup); >> +EXPORT_SYMBOL(rdma_addr_cleanup); >> diff --git a/drivers/infiniband/core/core_priv.h >> b/drivers/infiniband/core/core_priv.h >> index eab32215756b..e0a5187ea98c 100644 >> --- a/drivers/infiniband/core/core_priv.h >> +++ b/drivers/infiniband/core/core_priv.h >> @@ -137,4 +137,7 @@ static inline bool rdma_is_upper_dev_rcu(struct >> net_device *dev, >> return _upper == upper; >> } >> >> +int rdma_addr_init(void); >> +void rdma_addr_cleanup(void); >> + >> #endif /* _CORE_PRIV_H */ >> diff --git a/drivers/infiniband/core/device.c >> b/drivers/infiniband/core/device.c >> index 10979844026a..4fdf87a485cc 100644 >> --- a/drivers/infiniband/core/device.c >> +++ b/drivers/infiniband/core/device.c >> @@ -983,10 +983,18 @@ static int __init ib_core_init(void) >> goto err_sysfs; >> } >> >> + ret = rdma_addr_init(); >> + if (ret) { >> + pr_warn("Couldn't init IB address resolution module\n"); >> + goto err_ibnl; >> + } >> + >> ib_cache_setup(); >> >> return 0; >> >> +err_ibnl: >> + ibnl_cleanup(); >> err_sysfs: >> class_unregister(&ib_class); >> err_comp: >> @@ -999,6 +1007,7 @@ err: >> static void __exit ib_core_cleanup(void) >> { >> ib_cache_cleanup(); >> + rdma_addr_cleanup(); >> ibnl_cleanup(); >> class_unregister(&ib_class); >> destroy_workqueue(ib_comp_wq); >> >> >> >> -- >> Doug Ledford <dledford@redhat.com> >> GPG KeyID: 0E572FDD >> > I'm not sure it will solve the problem. it has been a while since > I've looked into how Linux handles modules so I might be wrong, but: > When we build a module that has to use a symbol from a different module > The symbol is unresolved (in has no address in the .ko file) the moment we load the module, > The kernel searches for the address of the symbol, maps the module into memory > and writes the correct address. > > Now in our case, verbs.c is part of ib_core.ko, and it uses rdma_addr_find_l2_eth_by_grh (which is in addr.c, part of ib_addr.ko) > So the moment we try to load ib_core we need to have ib_addr already in memory (because we need the address of rdma_addr_find_l2_eth_by_grh) > modprobe knows about that (because it sees there is a dependency between ib_core and ib_addr) so it tries to load ib_addr.ko first. Yep. > ib_addr uses ibnl, so it has an unresolved symbol, Yep. > which means ib_core.ko needs to be loaded before it, this is why we > have a cycle. > > After all the theoretical mambo jambo, I had to check it and this is what I got: > Using your code + my ibnl patches, the module_install step fails: > depmod: ERROR: Found 2 modules in dependency cycles! > depmod: ERROR: Cycle detected: ib_core -> ib_addr -> ib_core Unless you went back to your original first patch set where ib_netlink was a separate module, then the load order could be ib_netlink -> ib_addr -> ib_core and things would work. Possibly. Depends on whether the ib_netlink module has anything in it that references symbols in ib_core.
> -----Original Message----- > From: Doug Ledford [mailto:dledford@redhat.com] > Sent: Wednesday, May 18, 2016 5:17 PM > To: Mark Bloch <markb@mellanox.com>; leon@kernel.org > Cc: linux-rdma@vger.kernel.org > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > resolution module into core > [snip] > Unless you went back to your original first patch set where ib_netlink > was a separate module, then the load order could be ib_netlink -> > ib_addr -> ib_core and things would work. Possibly. Depends on whether > the ib_netlink module has anything in it that references symbols in ib_core. Yes, the first version did just that and worked, but then there were comments that we have too many modules and I shouldn't add another one. V0 adds a new kernel module (ib_netlink.ko ) but leaves ib_core and ib_addr modules unchanged. I also have a version with Leon's patch that eliminates ib_addr.ko and doesn't add a new module. Would you like me to submit a new version (with Leon's patch) or the review can continue with V0? > -- > Doug Ledford <dledford@redhat.com> > GPG KeyID: 0E572FDD > Mark -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2016 10:26 AM, Mark Bloch wrote: > > >> -----Original Message----- >> From: Doug Ledford [mailto:dledford@redhat.com] >> Sent: Wednesday, May 18, 2016 5:17 PM >> To: Mark Bloch <markb@mellanox.com>; leon@kernel.org >> Cc: linux-rdma@vger.kernel.org >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address >> resolution module into core >> > [snip] >> Unless you went back to your original first patch set where ib_netlink >> was a separate module, then the load order could be ib_netlink -> >> ib_addr -> ib_core and things would work. Possibly. Depends on whether >> the ib_netlink module has anything in it that references symbols in ib_core. > Yes, the first version did just that and worked, but then there were > comments that we have too many modules and I shouldn't add another one. > > V0 adds a new kernel module (ib_netlink.ko ) but leaves ib_core and ib_addr > modules unchanged. I also have a version with Leon's patch > that eliminates ib_addr.ko and doesn't add a new module. > > Would you like me to submit a new version (with Leon's patch) or the review can continue with V0? One of the things that has come about as a result of this conversation is that EL6 is the only distro to provide the reload capability. I have the ability to make sure it gets updated to account for the module change, so let's move forward with a version that removes ib_addr and moves it into the ib_core and also keep the new ib_netlink stuff in ib_core. So, fewer modules, not more.
On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: > moves it into the ib_core and also keep the new ib_netlink stuff in > ib_core. So, fewer modules, not more. What about getting rid of ib_sa as well so we can avoid that dynamic netlink registration patch too? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: >> moves it into the ib_core and also keep the new ib_netlink stuff in >> ib_core. So, fewer modules, not more. > > What about getting rid of ib_sa as well so we can avoid that dynamic > netlink registration patch too? That's fine too.
> -----Original Message----- > From: Doug Ledford [mailto:dledford@redhat.com] > Sent: Wednesday, May 18, 2016 8:59 PM > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; linux- > rdma@vger.kernel.org > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > resolution module into core > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: > >> moves it into the ib_core and also keep the new ib_netlink stuff in > >> ib_core. So, fewer modules, not more. > > > > What about getting rid of ib_sa as well so we can avoid that dynamic > > netlink registration patch too? > > That's fine too. Integrating ib_sa into ib_core also means to do the same for ib_mad. If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create the patches. Mark -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2016 02:28 PM, Mark Bloch wrote: > > >> -----Original Message----- >> From: Doug Ledford [mailto:dledford@redhat.com] >> Sent: Wednesday, May 18, 2016 8:59 PM >> To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> >> Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; linux- >> rdma@vger.kernel.org >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address >> resolution module into core >> >> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: >>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: >>>> moves it into the ib_core and also keep the new ib_netlink stuff in >>>> ib_core. So, fewer modules, not more. >>> >>> What about getting rid of ib_sa as well so we can avoid that dynamic >>> netlink registration patch too? >> >> That's fine too. > Integrating ib_sa into ib_core also means to do the same for ib_mad. > If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create the patches. Ira, Hal?
> On 05/18/2016 02:28 PM, Mark Bloch wrote: > > > > > >> -----Original Message----- > >> From: Doug Ledford [mailto:dledford@redhat.com] > >> Sent: Wednesday, May 18, 2016 8:59 PM > >> To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > >> Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; linux- > >> rdma@vger.kernel.org > >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > >> resolution module into core > >> > >> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > >>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: > >>>> moves it into the ib_core and also keep the new ib_netlink stuff in > >>>> ib_core. So, fewer modules, not more. > >>> > >>> What about getting rid of ib_sa as well so we can avoid that dynamic > >>> netlink registration patch too? > >> > >> That's fine too. > > Integrating ib_sa into ib_core also means to do the same for ib_mad. > > If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create > the patches. > > Ira, Hal? > I don't see a problem with that. Ira -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote: > > On 05/18/2016 02:28 PM, Mark Bloch wrote: > > > > > > > > >> -----Original Message----- > > >> From: Doug Ledford [mailto:dledford@redhat.com] > > >> Sent: Wednesday, May 18, 2016 8:59 PM > > >> To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > >> Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; linux- > > >> rdma@vger.kernel.org > > >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > > >> resolution module into core > > >> > > >> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > > >>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: > > >>>> moves it into the ib_core and also keep the new ib_netlink stuff in > > >>>> ib_core. So, fewer modules, not more. > > >>> > > >>> What about getting rid of ib_sa as well so we can avoid that dynamic > > >>> netlink registration patch too? > > >> > > >> That's fine too. > > > Integrating ib_sa into ib_core also means to do the same for ib_mad. > > > If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create > > the patches. > > > > Ira, Hal? > > > > I don't see a problem with that. Doug, Will it be acceptable to you if Mark base his patches on this assumption? Thanks. > > Ira >
On 5/23/2016 4:58 AM, Leon Romanovsky wrote: > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote: >>> On 05/18/2016 02:28 PM, Mark Bloch wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Doug Ledford [mailto:dledford@redhat.com] >>>>> Sent: Wednesday, May 18, 2016 8:59 PM >>>>> To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> >>>>> Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; linux- >>>>> rdma@vger.kernel.org >>>>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address >>>>> resolution module into core >>>>> >>>>> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: >>>>>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: >>>>>>> moves it into the ib_core and also keep the new ib_netlink stuff in >>>>>>> ib_core. So, fewer modules, not more. >>>>>> >>>>>> What about getting rid of ib_sa as well so we can avoid that dynamic >>>>>> netlink registration patch too? >>>>> >>>>> That's fine too. >>>> Integrating ib_sa into ib_core also means to do the same for ib_mad. >>>> If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create >>> the patches. >>> >>> Ira, Hal? >>> >> >> I don't see a problem with that. > > Doug, > > Will it be acceptable to you if Mark base his patches on this assumption? Yes.
> -----Original Message----- > From: Doug Ledford [mailto:dledford@redhat.com] > Sent: Monday, May 23, 2016 5:06 PM > To: leon@kernel.org; Weiny, Ira <ira.weiny@intel.com> > Cc: Mark Bloch <markb@mellanox.com>; Jason Gunthorpe > <jgunthorpe@obsidianresearch.com>; linux-rdma@vger.kernel.org; > hal.rosenstock@gmail.com > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > resolution module into core > > On 5/23/2016 4:58 AM, Leon Romanovsky wrote: > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote: > >>> On 05/18/2016 02:28 PM, Mark Bloch wrote: > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Doug Ledford [mailto:dledford@redhat.com] > >>>>> Sent: Wednesday, May 18, 2016 8:59 PM > >>>>> To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > >>>>> Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; linux- > >>>>> rdma@vger.kernel.org > >>>>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > >>>>> resolution module into core > >>>>> > >>>>> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > >>>>>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote: > >>>>>>> moves it into the ib_core and also keep the new ib_netlink stuff in > >>>>>>> ib_core. So, fewer modules, not more. > >>>>>> > >>>>>> What about getting rid of ib_sa as well so we can avoid that dynamic > >>>>>> netlink registration patch too? > >>>>> > >>>>> That's fine too. > >>>> Integrating ib_sa into ib_core also means to do the same for ib_mad. > >>>> If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll > create > >>> the patches. > >>> > >>> Ira, Hal? > >>> > >> > >> I don't see a problem with that. > > > > Doug, > > > > Will it be acceptable to you if Mark base his patches on this assumption? > > Yes. > Actually the new series is already posted, http://marc.info/?l=linux-rdma&m=146366717406108&w=2 if you can have a look at it'll be great. Mark -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2016-05-23 at 14:09 +0000, Mark Bloch wrote: > > > -----Original Message----- > > From: Doug Ledford [mailto:dledford@redhat.com] > > Sent: Monday, May 23, 2016 5:06 PM > > To: leon@kernel.org; Weiny, Ira <ira.weiny@intel.com> > > Cc: Mark Bloch <markb@mellanox.com>; Jason Gunthorpe > > <jgunthorpe@obsidianresearch.com>; linux-rdma@vger.kernel.org; > > hal.rosenstock@gmail.com > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > > resolution module into core > > > > On 5/23/2016 4:58 AM, Leon Romanovsky wrote: > > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote: > > > > > On 05/18/2016 02:28 PM, Mark Bloch wrote: > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Doug Ledford [mailto:dledford@redhat.com] > > > > > > > Sent: Wednesday, May 18, 2016 8:59 PM > > > > > > > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > > > > > > Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; > > > > > > > linux- > > > > > > > rdma@vger.kernel.org > > > > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate > > > > > > > IB address > > > > > > > resolution module into core > > > > > > > > > > > > > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > > > > > > > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford > > > > > > > > wrote: > > > > > > > > > moves it into the ib_core and also keep the new > > > > > > > > > ib_netlink stuff in > > > > > > > > > ib_core. So, fewer modules, not more. > > > > > > > > > > > > > > > > What about getting rid of ib_sa as well so we can avoid > > > > > > > > that dynamic > > > > > > > > netlink registration patch too? > > > > > > > > > > > > > > That's fine too. > > > > > > Integrating ib_sa into ib_core also means to do the same > > > > > > for ib_mad. > > > > > > If you agree with it (ib_sa & ib_mad becoming part of > > > > > > ib_core), I'll > > create > > > > > the patches. > > > > > > > > > > Ira, Hal? > > > > > Having just been through a development of a new HCA from scratch, I found it very useful to have the separation between the core IB functionality and what I perceive as clients of that core infrastructure (ib_mad and ib_sa in particular). The separation feels natural, and it allows a more piecemeal approach - a device can be loaded and tested with one WR at a time. Once ib_mad gets loaded, a lot of "traffic" is created and just about the whole HCA under development needs to work to support it. IMHO, if these "clients" gets joined with ib_core, the path for newcomers will be (even) steeper. I would appreciate if that particular separation can be kept. I am fine with the other merges, and it might as well be a good idea to join ib_mad and ib_sa, Thanks, Knut Omang (lead developer of Oracle's new IB HCA, soon to appear here..:-) ) > > > > I don't see a problem with that. > > > > > > Doug, > > > > > > Will it be acceptable to you if Mark base his patches on this > > > assumption? > > > > Yes. > > > > Actually the new series is already posted, > http://marc.info/?l=linux-rdma&m=146366717406108&w=2 > if you can have a look at it'll be great. > > Mark > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 30, 2016 at 07:33:14AM +0200, Knut Omang wrote: > On Mon, 2016-05-23 at 14:09 +0000, Mark Bloch wrote: > > > > > -----Original Message----- > > > From: Doug Ledford [mailto:dledford@redhat.com] > > > Sent: Monday, May 23, 2016 5:06 PM > > > To: leon@kernel.org; Weiny, Ira <ira.weiny@intel.com> > > > Cc: Mark Bloch <markb@mellanox.com>; Jason Gunthorpe > > > <jgunthorpe@obsidianresearch.com>; linux-rdma@vger.kernel.org; > > > hal.rosenstock@gmail.com > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > > > resolution module into core > > > > > > On 5/23/2016 4:58 AM, Leon Romanovsky wrote: > > > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote: > > > > > > On 05/18/2016 02:28 PM, Mark Bloch wrote: > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Doug Ledford [mailto:dledford@redhat.com] > > > > > > > > Sent: Wednesday, May 18, 2016 8:59 PM > > > > > > > > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > > > > > > > Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; > > > > > > > > linux- > > > > > > > > rdma@vger.kernel.org > > > > > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate > > > > > > > > IB address > > > > > > > > resolution module into core > > > > > > > > > > > > > > > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > > > > > > > > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford > > > > > > > > > wrote: > > > > > > > > > > moves it into the ib_core and also keep the new > > > > > > > > > > ib_netlink stuff in > > > > > > > > > > ib_core. So, fewer modules, not more. > > > > > > > > > > > > > > > > > > What about getting rid of ib_sa as well so we can avoid > > > > > > > > > that dynamic > > > > > > > > > netlink registration patch too? > > > > > > > > > > > > > > > > That's fine too. > > > > > > > Integrating ib_sa into ib_core also means to do the same > > > > > > > for ib_mad. > > > > > > > If you agree with it (ib_sa & ib_mad becoming part of > > > > > > > ib_core), I'll > > > create > > > > > > the patches. > > > > > > > > > > > > Ira, Hal? > > > > > > > > Having just been through a development of a new HCA from scratch, I > found it very useful to have the separation between the core IB > functionality and what I perceive as clients of that core > infrastructure (ib_mad and ib_sa in particular). > > The separation feels natural, and it allows a more piecemeal approach - > a device can be loaded and tested with one WR at a time. Once ib_mad > gets loaded, a lot of "traffic" is created and just about the whole HCA > under development needs to work to support it. Adding new device to IB is not an easy task and we clearly need to improve in that area, however these merges are targeted to the end-user of IB stack which constantly needs to load all these modules. For the easy development, you can simply revert them in your tree. Hope it helps. > > IMHO, if these "clients" gets joined with ib_core, the path for > newcomers will be (even) steeper. I would appreciate if that particular > separation can be kept. I am fine with the other merges, and it might > as well be a good idea to join ib_mad and ib_sa, > > Thanks, > Knut Omang > (lead developer of Oracle's new IB HCA, soon to appear here..:-) ) Terrific, We are eager to see it. > > > > > > I don't see a problem with that. > > > > > > > > Doug, > > > > > > > > Will it be acceptable to you if Mark base his patches on this > > > > assumption? > > > > > > Yes. > > > > > > > Actually the new series is already posted, > > http://marc.info/?l=linux-rdma&m=146366717406108&w=2 > > if you can have a look at it'll be great. > > > > Mark > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2016-05-30 at 09:33 +0300, Leon Romanovsky wrote: > On Mon, May 30, 2016 at 07:33:14AM +0200, Knut Omang wrote: > > On Mon, 2016-05-23 at 14:09 +0000, Mark Bloch wrote: > > > > > > > -----Original Message----- > > > > From: Doug Ledford [mailto:dledford@redhat.com] > > > > Sent: Monday, May 23, 2016 5:06 PM > > > > To: leon@kernel.org; Weiny, Ira <ira.weiny@intel.com> > > > > Cc: Mark Bloch <markb@mellanox.com>; Jason Gunthorpe > > > > <jgunthorpe@obsidianresearch.com>; linux-rdma@vger.kernel.org; > > > > hal.rosenstock@gmail.com > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address > > > > resolution module into core > > > > > > > > On 5/23/2016 4:58 AM, Leon Romanovsky wrote: > > > > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote: > > > > > > > On 05/18/2016 02:28 PM, Mark Bloch wrote: > > > > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Doug Ledford [mailto:dledford@redhat.com] > > > > > > > > > Sent: Wednesday, May 18, 2016 8:59 PM > > > > > > > > > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > > > > > > > > Cc: Mark Bloch <markb@mellanox.com>; leon@kernel.org; > > > > > > > > > linux- > > > > > > > > > rdma@vger.kernel.org > > > > > > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate > > > > > > > > > IB address > > > > > > > > > resolution module into core > > > > > > > > > > > > > > > > > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote: > > > > > > > > > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford > > > > > > > > > > wrote: > > > > > > > > > > > moves it into the ib_core and also keep the new > > > > > > > > > > > ib_netlink stuff in > > > > > > > > > > > ib_core. So, fewer modules, not more. > > > > > > > > > > > > > > > > > > > > What about getting rid of ib_sa as well so we can avoid > > > > > > > > > > that dynamic > > > > > > > > > > netlink registration patch too? > > > > > > > > > > > > > > > > > > That's fine too. > > > > > > > > Integrating ib_sa into ib_core also means to do the same > > > > > > > > for ib_mad. > > > > > > > > If you agree with it (ib_sa & ib_mad becoming part of > > > > > > > > ib_core), I'll > > > > create > > > > > > > the patches. > > > > > > > > > > > > > > Ira, Hal? > > > > > > > > > > > Having just been through a development of a new HCA from scratch, I > > found it very useful to have the separation between the core IB > > functionality and what I perceive as clients of that core > > infrastructure (ib_mad and ib_sa in particular). > > > > The separation feels natural, and it allows a more piecemeal approach - > > a device can be loaded and tested with one WR at a time. Once ib_mad > > gets loaded, a lot of "traffic" is created and just about the whole HCA > > under development needs to work to support it. > > Adding new device to IB is not an easy task and we clearly need to improve in > that area, however these merges are targeted to the end-user of IB stack which > constantly needs to load all these modules. In a well configured system, this should all happen automatically - this is why we have udev? End users should never have to struggle at this level. > For the easy development, you can simply revert them in your tree. :-) Development and debugging needs aside: Keeping separation between layers is still a good idea IMHO. > Hope it helps. > > > > > IMHO, if these "clients" gets joined with ib_core, the path for > > newcomers will be (even) steeper. I would appreciate if that particular > > separation can be kept. I am fine with the other merges, and it might > > as well be a good idea to join ib_mad and ib_sa, > > > > Thanks, > > Knut Omang > > (lead developer of Oracle's new IB HCA, soon to appear here..:-) ) > > Terrific, > We are eager to see it. Good things come to those who wait :-) Still playing catch-up with the activity here as is evident from my late comment in this thread.. Knut > > > > > > > > I don't see a problem with that. > > > > > > > > > > Doug, > > > > > > > > > > Will it be acceptable to you if Mark base his patches on this > > > > > assumption? > > > > > > > > Yes. > > > > > > > > > > Actually the new series is already posted, > > > http://marc.info/?l=linux-rdma&m=146366717406108&w=2 > > > if you can have a look at it'll be great. > > > > > > Mark > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > > in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 30, 2016 at 01:37:35PM +0200, Knut Omang wrote: > > > The separation feels natural, and it allows a more piecemeal approach - > > > a device can be loaded and tested with one WR at a time. Once ib_mad > > > gets loaded, a lot of "traffic" is created and just about the whole HCA > > > under development needs to work to support it. > > > > Adding new device to IB is not an easy task and we clearly need to improve in > > that area, however these merges are targeted to the end-user of IB stack which > > constantly needs to load all these modules. > > In a well configured system, this should all happen automatically - > this is why we have udev? End users should never have to struggle at > this level. > > > For the easy development, you can simply revert them in your tree. > > :-) > Development and debugging needs aside: Keeping separation between > layers is still a good idea IMHO. We tried it for the first version of IB router patches which required conversion ib_netlink to be a module and found ourselves with bazillion little modules for every file in IB core.
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index 337353d86cfa..fda808546e0e 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -634,7 +634,17 @@ static struct notifier_block nb = { .notifier_call = netevent_callback }; -static int __init addr_init(void) +/* + * This is a little unorthodox. We export our init function so that the + * ib_core module can call it at the right time after the other modules + * we depend on have registered. We do this solely because there are some + * user space init scripts that expect ib_addr to be loaded prior to ib_core + * and due to recent changes, we need the ib netlink code brought up before + * we bring this code up and that code is now in ib_core. When the older + * SysV init script systems that have these init scripts are dead and gone, + * we can revisit this and possibly just include addr.c in ib_core. + */ +int rdma_addr_init(void) { addr_wq = create_singlethread_workqueue("ib_addr"); if (!addr_wq) @@ -644,13 +654,12 @@ static int __init addr_init(void) rdma_addr_register_client(&self); return 0; } +EXPORT_SYMBOL(rdma_addr_init); -static void __exit addr_cleanup(void) +void rdma_addr_cleanup(void) { rdma_addr_unregister_client(&self); unregister_netevent_notifier(&nb); destroy_workqueue(addr_wq); } - -module_init(addr_init); -module_exit(addr_cleanup); +EXPORT_SYMBOL(rdma_addr_cleanup); diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h index eab32215756b..e0a5187ea98c 100644 --- a/drivers/infiniband/core/core_priv.h +++ b/drivers/infiniband/core/core_priv.h @@ -137,4 +137,7 @@ static inline bool rdma_is_upper_dev_rcu(struct net_device *dev, return _upper == upper; } +int rdma_addr_init(void); +void rdma_addr_cleanup(void); + #endif /* _CORE_PRIV_H */ diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 10979844026a..4fdf87a485cc 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -983,10 +983,18 @@ static int __init ib_core_init(void) goto err_sysfs; } + ret = rdma_addr_init(); + if (ret) { + pr_warn("Couldn't init IB address resolution module\n"); + goto err_ibnl; + } + ib_cache_setup(); return 0; +err_ibnl: + ibnl_cleanup(); err_sysfs: class_unregister(&ib_class); err_comp: @@ -999,6 +1007,7 @@ err: static void __exit ib_core_cleanup(void) { ib_cache_cleanup(); + rdma_addr_cleanup(); ibnl_cleanup(); class_unregister(&ib_class); destroy_workqueue(ib_comp_wq);