Re: [PATCH for-next V6 05/10] IB/core: Add RoCE GID table management

Jason Gunthorpe
2015-07-17T19:02:45Z

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