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
> +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.
Next is the READ_ONCE fencing. I think it is totally unnecessary.
Patch #4 does this:
down_write(&lists_rwsem);
list_del(&device->core_list);
up_write(&lists_rwsem);
list_for_each_entry_reverse(client, &client_list, list)
if (client->remove)
client->remove(device);
So, by the time we get to gid_table_client_cleanup_one, it is no
longer possible for ib_enum_all_roce_netdevs to use the ib_device we
are removing (it is taken off the core_list).
Since all the queued work calls ib_enum_all_roce_netdevs, it is
impossibile for something like ib_cache_gid_add to be called from the
work queue with the ib_dev under removal.
In fact, even the flush_work is not needed because of how lists_rwsem
is being used: we can not remove something from the core list until
there are no ib_enum_all_roce_netdevs callbacks running.
Also, did you notice the double flush of the work queue? One is
enough:
static void ib_cache_cleanup_one(struct ib_device *device)
{
ib_unregister_event_handler(&device->cache.event_handler);
flush_workqueue(ib_wq);
gid_table_client_cleanup_one(device);
static void gid_table_client_cleanup_one(struct ib_device *ib_dev)
{
flush_workqueue(ib_wq);
No other locking problems screamed out at me, but it is a big patch,
and I have't looked closely at all of it.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
