On Thu, Mar 29, 2018 at 05:36:37PM -0600, Parav Pandit wrote:
> Hi Jason,
>
> > From: Jason Gunthorpe
> > Sent: Thursday, March 29, 2018 4:44 PM
> > To: linux-rdma@xxxxxxxxxxxxxxx
> > Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; Mark Bloch
> > <markb@xxxxxxxxxxxx>; Parav Pandit <parav@xxxxxxxxxxxx>
> > Subject: [RFC PATCH] IB/core: Simplify locking around data_vec
> >
> > Instead o using a complex scheme where we drop and re-acquire rwlock, switch
> > to a much simpler dual locking pattern where the sleepable lock and the spinlock
> > are held by all writers, while all readers only grab the spinlock.
> >
> > GID_TABLE_ENTRY_INVALID is used as the fence protected by that lock.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > drivers/infiniband/core/cache.c | 73 ++++++++++++++---------------------------
> > 1 file changed, 24 insertions(+), 49 deletions(-)
> >
> > This is based off Parav's latest series.
> >
> > What do you think Parav? Can you give it some test if you like it?
> >
> > I only added two new mutex(table->locks)'s:
> > - cleanup_gid_table_port(). This is only called during add/remove one
> > device which is a sleepable context
> Yes. This was in the TODO list, after this series. Thiw as good enough for the feature to progress.
>
> > - ib_cache_update() This is only called from a work queue, so
> > it should be fine
> No. this is not fine.
> Mutex lo replacement is done under holding silly pkey cache lock.
> write_lock_irq(&device->cache.lock);
> [..]
> mutex_lock.
Yikes! Indpendent of anything else that seems to be a dumb locking
ordering.
Easy to fix, just move the add_modify_gid outside of the
device->cache.lock, no reason I have seen for that locking order,
right?
@@ -1161,29 +1136,26 @@ static void ib_cache_update(struct ib_device *device,
goto err;
}
}
- }
-
- write_lock_irq(&device->cache.lock);
-
- old_pkey_cache = device->cache.ports[port -
- rdma_start_port(device)].pkey;
- device->cache.ports[port - rdma_start_port(device)].pkey = pkey_cache;
- if (!use_roce_gid_table) {
gid_attr.device = device;
gid_attr.port_num = port;
- write_lock(&table->rwlock);
+ mutex_lock(&table->lock);
for (i = 0; i < gid_cache->table_len; i++) {
gid_attr.index = i;
add_modify_gid(table, gid_cache->table + i, &gid_attr);
}
- write_unlock(&table->rwlock);
+ mutex_unlock(&table->lock);
}
+ write_lock_irq(&device->cache.lock);
+
+ old_pkey_cache = device->cache.ports[port -
+ rdma_start_port(device)].pkey;
+
+ device->cache.ports[port - rdma_start_port(device)].pkey = pkey_cache;
device->cache.ports[port - rdma_start_port(device)].lmc = tprops->lmc;
device->cache.ports[port - rdma_start_port(device)].port_state =
tprops->state;
-
> I already have patch that clean up this three time check of roce
> port property in single function along with this lock. Yesterday
> night I queued to Dan for internal review.
Yes, please tidy that function, what a terrible piece of code.
> I have next series based on this series, so I prefer to merge this
> suggested changed in v1 along with ib_cache_update cleanup patch. I
> split into two patches because it was possible and this patch was
> big enough.
This patch is big enough, but you can add a few more patches to this
series no problem. Can you do this one and your cleanup?
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
