| Message ID | CAMGffE=gytHRs+SVOmNPCkWX6tZ9hkeFznqeQQs-TFJMMJi97g@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi, On 12/12/2016 1:43 PM, Jinpu Wang wrote: > From fc80c1730d94a38934c5c60f9b9561745723dfd9 Mon Sep 17 00:00:00 2001 > From: Jack Wang <jinpu.wang@profitbricks.com> > Date: Mon, 12 Dec 2016 09:52:42 +0100 > Subject: [PATCH 1/4] IB/core: add port state cache > > We need a port state cache in ib_core, later we will use in rdma_cm. > > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> > Reviewed-by: Michael Wang <yun.wang@profitbricks.com> > --- > drivers/infiniband/core/cache.c | 9 ++++++++- > include/rdma/ib_verbs.h | 1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > index 1a2984c..025db27 100644 > --- a/drivers/infiniband/core/cache.c > +++ b/drivers/infiniband/core/cache.c > @@ -1109,6 +1109,8 @@ static void ib_cache_update(struct ib_device *device, > } > > device->cache.lmc_cache[port - rdma_start_port(device)] = tprops->lmc; > + device->cache.port_state_cache[port - rdma_start_port(device)] = > + tprops->state; > > write_unlock_irq(&device->cache.lock); > > @@ -1168,7 +1170,11 @@ int ib_cache_setup_one(struct ib_device *device) > (rdma_end_port(device) - > rdma_start_port(device) + 1), > GFP_KERNEL); > - if (!device->cache.pkey_cache || > + device->cache.port_state_cache = kmalloc(sizeof > *device->cache.port_state_cache * > + (rdma_end_port(device) - > + rdma_start_port(device) + 1), > + GFP_KERNEL); > + if (!device->cache.pkey_cache || !device->cache.port_state_cache || Looking at the code, I see we can leak memory here. if pkey_cache is allocated but lms_cache is not, we will return without freeing pkey_cache (or the other way around) your code adds port_state_cache to the mix but with the same issue. I guess that should be fixed. > !device->cache.lmc_cache) { > pr_warn("Couldn't allocate cache for %s\n", device->name); > return -ENOMEM; > @@ -1213,6 +1219,7 @@ void ib_cache_release_one(struct ib_device *device) > gid_table_release_one(device); > kfree(device->cache.pkey_cache); > kfree(device->cache.lmc_cache); > + kfree(device->cache.port_state_cache); > } > > void ib_cache_cleanup_one(struct ib_device *device) > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 5ad43a4..a167ae0 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1761,6 +1761,7 @@ struct ib_cache { > struct ib_pkey_cache **pkey_cache; > struct ib_gid_table **gid_cache; > u8 *lmc_cache; > + enum ib_port_state *port_state_cache; > }; > > struct ib_dma_mapping_ops { > 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, Dec 12, 2016 at 1:11 PM, Mark Bloch <markb@mellanox.com> wrote: > Hi, > > On 12/12/2016 1:43 PM, Jinpu Wang wrote: >> From fc80c1730d94a38934c5c60f9b9561745723dfd9 Mon Sep 17 00:00:00 2001 >> From: Jack Wang <jinpu.wang@profitbricks.com> >> Date: Mon, 12 Dec 2016 09:52:42 +0100 >> Subject: [PATCH 1/4] IB/core: add port state cache >> >> We need a port state cache in ib_core, later we will use in rdma_cm. >> >> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> >> Reviewed-by: Michael Wang <yun.wang@profitbricks.com> >> --- >> drivers/infiniband/core/cache.c | 9 ++++++++- >> include/rdma/ib_verbs.h | 1 + >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >> index 1a2984c..025db27 100644 >> --- a/drivers/infiniband/core/cache.c >> +++ b/drivers/infiniband/core/cache.c >> @@ -1109,6 +1109,8 @@ static void ib_cache_update(struct ib_device *device, >> } >> >> device->cache.lmc_cache[port - rdma_start_port(device)] = tprops->lmc; >> + device->cache.port_state_cache[port - rdma_start_port(device)] = >> + tprops->state; >> >> write_unlock_irq(&device->cache.lock); >> >> @@ -1168,7 +1170,11 @@ int ib_cache_setup_one(struct ib_device *device) >> (rdma_end_port(device) - >> rdma_start_port(device) + 1), >> GFP_KERNEL); >> - if (!device->cache.pkey_cache || >> + device->cache.port_state_cache = kmalloc(sizeof >> *device->cache.port_state_cache * >> + (rdma_end_port(device) - >> + rdma_start_port(device) + 1), >> + GFP_KERNEL); >> + if (!device->cache.pkey_cache || !device->cache.port_state_cache || > Looking at the code, I see we can leak memory here. > if pkey_cache is allocated but lms_cache is not, we will return without freeing pkey_cache (or the other way around) > your code adds port_state_cache to the mix but with the same issue. > I guess that should be fixed. > Hi Mark, Not really. The release was done by ib_dealloc_device() when put the last ref of kobj, dev_release -> ib_device_release -> ib_cache_release_one, driver was supposed to use it to destroy the device when register failed. Cheers, Jinpu >> !device->cache.lmc_cache) { >> pr_warn("Couldn't allocate cache for %s\n", device->name); >> return -ENOMEM; >> @@ -1213,6 +1219,7 @@ void ib_cache_release_one(struct ib_device *device) >> gid_table_release_one(device); >> kfree(device->cache.pkey_cache); >> kfree(device->cache.lmc_cache); >> + kfree(device->cache.port_state_cache); >> } >> >> void ib_cache_cleanup_one(struct ib_device *device) >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index 5ad43a4..a167ae0 100644 >> --- a/include/rdma/ib_verbs.h >> +++ b/include/rdma/ib_verbs.h >> @@ -1761,6 +1761,7 @@ struct ib_cache { >> struct ib_pkey_cache **pkey_cache; >> struct ib_gid_table **gid_cache; >> u8 *lmc_cache; >> + enum ib_port_state *port_state_cache; >> }; >> >> struct ib_dma_mapping_ops { >> > > Mark.
On Mon, Dec 12, 2016 at 01:31:11PM +0100, Jinpu Wang wrote: > On Mon, Dec 12, 2016 at 1:11 PM, Mark Bloch <markb@mellanox.com> wrote: > > Hi, > > > > On 12/12/2016 1:43 PM, Jinpu Wang wrote: > >> From fc80c1730d94a38934c5c60f9b9561745723dfd9 Mon Sep 17 00:00:00 2001 > >> From: Jack Wang <jinpu.wang@profitbricks.com> > >> Date: Mon, 12 Dec 2016 09:52:42 +0100 > >> Subject: [PATCH 1/4] IB/core: add port state cache > >> > >> We need a port state cache in ib_core, later we will use in rdma_cm. > >> > >> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> > >> Reviewed-by: Michael Wang <yun.wang@profitbricks.com> > >> --- > >> drivers/infiniband/core/cache.c | 9 ++++++++- > >> include/rdma/ib_verbs.h | 1 + > >> 2 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > >> index 1a2984c..025db27 100644 > >> --- a/drivers/infiniband/core/cache.c > >> +++ b/drivers/infiniband/core/cache.c > >> @@ -1109,6 +1109,8 @@ static void ib_cache_update(struct ib_device *device, > >> } > >> > >> device->cache.lmc_cache[port - rdma_start_port(device)] = tprops->lmc; > >> + device->cache.port_state_cache[port - rdma_start_port(device)] = > >> + tprops->state; > >> > >> write_unlock_irq(&device->cache.lock); > >> > >> @@ -1168,7 +1170,11 @@ int ib_cache_setup_one(struct ib_device *device) > >> (rdma_end_port(device) - > >> rdma_start_port(device) + 1), > >> GFP_KERNEL); > >> - if (!device->cache.pkey_cache || > >> + device->cache.port_state_cache = kmalloc(sizeof > >> *device->cache.port_state_cache * > >> + (rdma_end_port(device) - > >> + rdma_start_port(device) + 1), > >> + GFP_KERNEL); > >> + if (!device->cache.pkey_cache || !device->cache.port_state_cache || > > Looking at the code, I see we can leak memory here. > > if pkey_cache is allocated but lms_cache is not, we will return without freeing pkey_cache (or the other way around) > > your code adds port_state_cache to the mix but with the same issue. > > I guess that should be fixed. > > > Hi Mark, > > Not really. The release was done by ib_dealloc_device() when put the > last ref of kobj, > dev_release -> ib_device_release -> ib_cache_release_one, driver was supposed > to use it to destroy the device when register failed. This is really odd though. Ira > > Cheers, > Jinpu > > >> !device->cache.lmc_cache) { > >> pr_warn("Couldn't allocate cache for %s\n", device->name); > >> return -ENOMEM; > >> @@ -1213,6 +1219,7 @@ void ib_cache_release_one(struct ib_device *device) > >> gid_table_release_one(device); > >> kfree(device->cache.pkey_cache); > >> kfree(device->cache.lmc_cache); > >> + kfree(device->cache.port_state_cache); > >> } > >> > >> void ib_cache_cleanup_one(struct ib_device *device) > >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > >> index 5ad43a4..a167ae0 100644 > >> --- a/include/rdma/ib_verbs.h > >> +++ b/include/rdma/ib_verbs.h > >> @@ -1761,6 +1761,7 @@ struct ib_cache { > >> struct ib_pkey_cache **pkey_cache; > >> struct ib_gid_table **gid_cache; > >> u8 *lmc_cache; > >> + enum ib_port_state *port_state_cache; > >> }; > >> > >> struct ib_dma_mapping_ops { > >> > > > > Mark. > > > > -- > Jinpu Wang > Linux Kernel Developer > > ProfitBricks GmbH > Greifswalder Str. 207 > D - 10405 Berlin > > Tel: +49 30 577 008 042 > Fax: +49 30 577 008 299 > Email: jinpu.wang@profitbricks.com > URL: https://www.profitbricks.de > > Sitz der Gesellschaft: Berlin > Registergericht: Amtsgericht Charlottenburg, HRB 125506 B > Geschäftsführer: Achim Weiss > -- > 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 Fri, Dec 16, 2016 at 12:17 AM, ira.weiny <ira.weiny@intel.com> wrote: > On Mon, Dec 12, 2016 at 01:31:11PM +0100, Jinpu Wang wrote: >> On Mon, Dec 12, 2016 at 1:11 PM, Mark Bloch <markb@mellanox.com> wrote: >> > Hi, >> > >> > On 12/12/2016 1:43 PM, Jinpu Wang wrote: >> >> From fc80c1730d94a38934c5c60f9b9561745723dfd9 Mon Sep 17 00:00:00 2001 >> >> From: Jack Wang <jinpu.wang@profitbricks.com> >> >> Date: Mon, 12 Dec 2016 09:52:42 +0100 >> >> Subject: [PATCH 1/4] IB/core: add port state cache >> >> >> >> We need a port state cache in ib_core, later we will use in rdma_cm. >> >> >> >> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> >> >> Reviewed-by: Michael Wang <yun.wang@profitbricks.com> >> >> --- >> >> drivers/infiniband/core/cache.c | 9 ++++++++- >> >> include/rdma/ib_verbs.h | 1 + >> >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >> >> index 1a2984c..025db27 100644 >> >> --- a/drivers/infiniband/core/cache.c >> >> +++ b/drivers/infiniband/core/cache.c >> >> @@ -1109,6 +1109,8 @@ static void ib_cache_update(struct ib_device *device, >> >> } >> >> >> >> device->cache.lmc_cache[port - rdma_start_port(device)] = tprops->lmc; >> >> + device->cache.port_state_cache[port - rdma_start_port(device)] = >> >> + tprops->state; >> >> >> >> write_unlock_irq(&device->cache.lock); >> >> >> >> @@ -1168,7 +1170,11 @@ int ib_cache_setup_one(struct ib_device *device) >> >> (rdma_end_port(device) - >> >> rdma_start_port(device) + 1), >> >> GFP_KERNEL); >> >> - if (!device->cache.pkey_cache || >> >> + device->cache.port_state_cache = kmalloc(sizeof >> >> *device->cache.port_state_cache * >> >> + (rdma_end_port(device) - >> >> + rdma_start_port(device) + 1), >> >> + GFP_KERNEL); >> >> + if (!device->cache.pkey_cache || !device->cache.port_state_cache || >> > Looking at the code, I see we can leak memory here. >> > if pkey_cache is allocated but lms_cache is not, we will return without freeing pkey_cache (or the other way around) >> > your code adds port_state_cache to the mix but with the same issue. >> > I guess that should be fixed. >> > >> Hi Mark, >> >> Not really. The release was done by ib_dealloc_device() when put the >> last ref of kobj, >> dev_release -> ib_device_release -> ib_cache_release_one, driver was supposed >> to use it to destroy the device when register failed. > > This is really odd though. > > Ira Hi Ira, I agree it's not straight forward indeed. But this is common in a lot code path if use kobj->release function. On the other side, this is clear not a blocker for this bugfix patchset, could anyone of you give a feedback to let it move forward? Thanks
On Mon, 2016-12-12 at 12:43 +0100, Jinpu Wang wrote: > From fc80c1730d94a38934c5c60f9b9561745723dfd9 Mon Sep 17 00:00:00 > 2001 > From: Jack Wang <jinpu.wang@profitbricks.com> > Date: Mon, 12 Dec 2016 09:52:42 +0100 > Subject: [PATCH 1/4] IB/core: add port state cache > > We need a port state cache in ib_core, later we will use in rdma_cm. All of these patches were mangled. Their tabs were converted to spaces, they had line wraps where they shouldn't, etc. Please don't send them using an email program, it obviously isn't working in your case. I strongly recommend using "git send-email" to send the series. Before resending, add a comment to the first about the memory being freed during the release operation, that will forestall any more questions about memory leaks. Also capture Sean Hefty's ack on all of your patches. I'm ready to take them, I just need you to rework them. Thanks. > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> > Reviewed-by: Michael Wang <yun.wang@profitbricks.com> > --- > drivers/infiniband/core/cache.c | 9 ++++++++- > include/rdma/ib_verbs.h | 1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cache.c > b/drivers/infiniband/core/cache.c > index 1a2984c..025db27 100644 > --- a/drivers/infiniband/core/cache.c > +++ b/drivers/infiniband/core/cache.c > @@ -1109,6 +1109,8 @@ static void ib_cache_update(struct ib_device > *device, > } > > device->cache.lmc_cache[port - rdma_start_port(device)] = > tprops->lmc; > + device->cache.port_state_cache[port - rdma_start_port(device)] = > + tprops->state; > > write_unlock_irq(&device->cache.lock); > > @@ -1168,7 +1170,11 @@ int ib_cache_setup_one(struct ib_device > *device) > (rdma_end_port(device) - > rdma_start_port(device) + 1), > GFP_KERNEL); > - if (!device->cache.pkey_cache || > + device->cache.port_state_cache = kmalloc(sizeof > *device->cache.port_state_cache * > + (rdma_end_port(device) - > + rdma_start_port(device) + 1), > + GFP_KERNEL); > + if (!device->cache.pkey_cache || !device->cache.port_state_cache > || > !device->cache.lmc_cache) { > pr_warn("Couldn't allocate cache for %s\n", device->name); > return -ENOMEM; > @@ -1213,6 +1219,7 @@ void ib_cache_release_one(struct ib_device > *device) > gid_table_release_one(device); > kfree(device->cache.pkey_cache); > kfree(device->cache.lmc_cache); > + kfree(device->cache.port_state_cache); > } > > void ib_cache_cleanup_one(struct ib_device *device) > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 5ad43a4..a167ae0 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1761,6 +1761,7 @@ struct ib_cache { > struct ib_pkey_cache **pkey_cache; > struct ib_gid_table **gid_cache; > u8 *lmc_cache; > + enum ib_port_state *port_state_cache; > }; > > struct ib_dma_mapping_ops { > -- > 2.7.4 > >
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index 1a2984c..025db27 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -1109,6 +1109,8 @@ static void ib_cache_update(struct ib_device *device, } device->cache.lmc_cache[port - rdma_start_port(device)] = tprops->lmc; + device->cache.port_state_cache[port - rdma_start_port(device)] = + tprops->state; write_unlock_irq(&device->cache.lock); @@ -1168,7 +1170,11 @@ int ib_cache_setup_one(struct ib_device *device) (rdma_end_port(device) - rdma_start_port(device) + 1), GFP_KERNEL); - if (!device->cache.pkey_cache || + device->cache.port_state_cache = kmalloc(sizeof *device->cache.port_state_cache * + (rdma_end_port(device) - + rdma_start_port(device) + 1), + GFP_KERNEL); + if (!device->cache.pkey_cache || !device->cache.port_state_cache || !device->cache.lmc_cache) { pr_warn("Couldn't allocate cache for %s\n", device->name); return -ENOMEM; @@ -1213,6 +1219,7 @@ void ib_cache_release_one(struct ib_device *device) gid_table_release_one(device); kfree(device->cache.pkey_cache); kfree(device->cache.lmc_cache); + kfree(device->cache.port_state_cache); } void ib_cache_cleanup_one(struct ib_device *device) diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 5ad43a4..a167ae0 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1761,6 +1761,7 @@ struct ib_cache { struct ib_pkey_cache **pkey_cache; struct ib_gid_table **gid_cache; u8 *lmc_cache; + enum ib_port_state *port_state_cache; }; struct ib_dma_mapping_ops {