On Tue, Jul 21, 2015 at 11:42:29AM +0300, Matan Barak wrote:
> On Fri, Jul 17, 2015 at 10:02 PM, Jason Gunthorpe
> <[email protected]> wrote:
> > On Wed, Jun 24, 2015 at 03:59:21PM +0300, Matan Barak wrote:
> >> +
> >> + /* in rdma_cap_roce_gid_table, this funciton should be protected by a
> >> + * sleep-able lock.
> >> + */
> >> + write_lock(&table->data_vec[ix].lock);
> >
> > I'm having a hard time understanding this comment
> >
>
> The same function is used for both RoCE and IB. Since RoCE unlocks the
> rwlock, calls the vendor's callback and
> locks it again. If two write_gid(s) are executed simultaneously, you
> need to protect them from writing to the
> same entry. The vendor's callback might sleep so we require a sleep-able lock.
You might want to retouch that comment a bit..
> >> +int ib_cache_gid_add(struct ib_device *ib_dev, u8 port,
> >> + union ib_gid *gid, struct ib_gid_attr *attr)
> >> +{
> >> + struct ib_gid_table **ports_table =
> >> + READ_ONCE(ib_dev->cache.gid_cache);
> >> + /* all table reads depend on ports_table, no need for smp_rmb() */
> >> + if (!ports_table)
> >> + return -EOPNOTSUPP;
> >
> > This common pattern does look genuinely odd...
> >
> > The gid_cache is part of the common API, it really shouldn't be kfree'd
> > while held struct ib_devices are around. The kfree for all the cache.c
> > stuff should probably be called from ib_device_release, not from the
> > client release.
> >
> > That is actually something the current code does that is possibly
> > wrong. It is trivially fixed by moving all the kfrees to
> > ib_device_release.
> >
>
> But cache as a whole is implemented as a client (cache_client).
> Isn't it a bit odd to free a client in ib_device_release?
The cache is part of the core code, it exports core APIs that need to
continue working as long as a ib_device object exists.
It abuses the client stuff to hook registration, that doesn't make it
'wholly implemented as a client'
It is not odd for core code to free its memory in ib_device_release.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
