Re: [PATCH 1/4] IB/core: add port state cache

["ira.weiny" <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 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-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> >> 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-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> >> Reviewed-by: Michael Wang <yun.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> >> ---
> >>  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-EIkl63zCoXaH+58JC4qpiA@public.gmane.org
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html