> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Tuesday, October 29, 2019 10:55 AM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA
> mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> <danielj@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next v1 1/2] IB/core: Let IB core distribute cache
> update events
>
> On Tue, Oct 29, 2019 at 03:47:42PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > Sent: Tuesday, October 29, 2019 10:34 AM
> > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>;
> RDMA
> > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> > > <danielj@xxxxxxxxxxxx>
> > > Subject: Re: [PATCH rdma-next v1 1/2] IB/core: Let IB core
> > > distribute cache update events
> > >
> > > On Tue, Oct 29, 2019 at 03:32:46PM +0000, Parav Pandit wrote:
> > >
> > > > > > +/**
> > > > > > + * ib_dispatch_event - Dispatch an asynchronous event
> > > > > > + * @event:Event to dispatch
> > > > > > + *
> > > > > > + * Low-level drivers must call ib_dispatch_event() to
> > > > > > +dispatch the
> > > > > > + * event to all registered event handlers when an
> > > > > > +asynchronous event
> > > > > > + * occurs.
> > > > > > + */
> > > > > > +void ib_dispatch_event(struct ib_event *event) {
> > > > > > + ib_enqueue_cache_update_event(event);
> > > > > > +}
> > > > > > EXPORT_SYMBOL(ib_dispatch_event);
> > > > >
> > > > > Why not just move this into cache.c?
> > > > >
> > > > Same thought same to me when I had to add one liner call.
> > > > However the issue was device.c has the code for the event
> > > registration/unregistration and calling the handlers unrelated to cache.
> > > > So moving ib_dispatch_event() to cache.c looked incorrect to me.
> > >
> > > Well, maybe we can move the wq code from the cache.c into here?
> >
> > I prefer to keep all cache specific code in cache.c because there is
> > where we flush the ib_wq, queue work there.
>
> I think that is because the cache is not subscribing to a notifier, it really should,
> then things order properly and the flush is not needed.
>
Cache shouldn't subscribe to the notifier, that is the race condition issue.
With this fix, cache invokes the notifier chain in wq context after all cache state is synced.
More below.
> > > It looks just as incorrect to have the one line call
> >
> > No, its not incorrect. Because device.c provides functionality to
> > register/unregister handler and dispatch event. While cache layer
> > deals with cache updates etc, and uses wq service provided device.c If
> > we rename ib_dispatch_event() to ib_dispatch_cache_event() it make
> > sense to move to cache.c However we have non cache specific events
> > there too, so current code split between cache.c and device.c looks
> > appropriate.
>
> It always looks bad to have a single line function call like that, especially just for
> spurious reasons like file placement or functioning naming. It shows something
> is being done wrong
>
Actually this v1 version of the patch is incorrect.
Even though we don't have cache update event, patch calls the cache work.
My previous version was correct.
But improved version is below that addressed your comment.
I took care to get rid of one liner function too.
Inlined the patch below for review purpose. Will send through Leon's queue.
This fix exposed another dormant bug of mlx4 driver. So will send it together as pre-patch to this patch.
>From 97c8a3050471e1d27fdd7f529784d1fa0c871d43 Mon Sep 17 00:00:00 2001
From: Parav Pandit <parav@xxxxxxxxxxxx>
Date: Thu, 26 Sep 2019 15:55:26 -0500
Subject: [PATCH] IB/core: Let IB core distribute cache update events
Currently when low level driver notifies Pkey, GID, port change events,
they are notified to the registered handlers in the order they are
registered.
IB core and other ULPs such as IPoIB are interested in GID, LID, Pkey
change events.
Since all GID query done by ULPs is serviced by IB core, in below flow
when GID change event occurs, IB core is yet to update the GID cache
when IPoIB queries the GID, resulting into not updating IPoIB address.
mlx5_ib_handle_event()
ib_dispatch_event()
ib_cache_event()
queue_work() -> slow cache update
[..]
ipoib_event()
queue_work()
[..]
work handler
ipoib_ib_dev_flush_light()
__ipoib_ib_dev_flush()
ipoib_dev_addr_changed_valid()
rdma_query_gid() <- Returns old GID, cache not updated.
Hence, all events which require cache update are handled first by the IB
core. Once cache update work is completed, IB core distributes the event
to subscriber clients.
---
Changelog:
v1->v2:
- Fixed not to call cache update when it is not a cache related event
- Always invoke notifier handler in workqueue context
- Removed incorrect comment about caller context of handler as it needs
some more work
- Replaced handler spin lock with rwsemaphore to make use of workqueue
context
- Moved ib_dispatch_event to cache.c as it distributes all events in wq
managed mostly by cache.c
Fixes: f35faa4ba956 ("IB/core: Simplify ib_query_gid to always refer to cache")
Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
---
drivers/infiniband/core/cache.c | 121 +++++++++++++++++-----------
drivers/infiniband/core/core_priv.h | 1 +
drivers/infiniband/core/device.c | 11 +--
include/rdma/ib_verbs.h | 3 +-
4 files changed, 78 insertions(+), 58 deletions(-)
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 00fb3eacda19..65b10efca2b8 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -51,9 +51,8 @@ struct ib_pkey_cache {
struct ib_update_work {
struct work_struct work;
- struct ib_device *device;
- u8 port_num;
- bool enforce_security;
+ struct ib_event event;
+ bool enforce_security;
};
union ib_gid zgid;
@@ -130,7 +129,7 @@ static void dispatch_gid_change_event(struct ib_device *ib_dev, u8 port)
event.element.port_num = port;
event.event = IB_EVENT_GID_CHANGE;
- ib_dispatch_event(&event);
+ ib_dispatch_event_clients(&event);
}
static const char * const gid_type_str[] = {
@@ -1387,9 +1386,8 @@ static int config_non_roce_gid_cache(struct ib_device *device,
return ret;
}
-static void ib_cache_update(struct ib_device *device,
- u8 port,
- bool enforce_security)
+static int
+ib_cache_update(struct ib_device *device, u8 port, bool enforce_security)
{
struct ib_port_attr *tprops = NULL;
struct ib_pkey_cache *pkey_cache = NULL, *old_pkey_cache;
@@ -1397,11 +1395,11 @@ static void ib_cache_update(struct ib_device *device,
int ret;
if (!rdma_is_port_valid(device, port))
- return;
+ return -EINVAL;
tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
if (!tprops)
- return;
+ return -ENOMEM;
ret = ib_query_port(device, port, tprops);
if (ret) {
@@ -1419,8 +1417,10 @@ static void ib_cache_update(struct ib_device *device,
pkey_cache = kmalloc(struct_size(pkey_cache, table,
tprops->pkey_tbl_len),
GFP_KERNEL);
- if (!pkey_cache)
+ if (!pkey_cache) {
+ ret = -ENOMEM;
goto err;
+ }
pkey_cache->table_len = tprops->pkey_tbl_len;
@@ -1452,50 +1452,84 @@ static void ib_cache_update(struct ib_device *device,
kfree(old_pkey_cache);
kfree(tprops);
- return;
+ return 0;
err:
kfree(pkey_cache);
kfree(tprops);
+ return ret;
+}
+
+static void ib_cache_event_task(struct work_struct *_work)
+{
+ struct ib_update_work *work =
+ container_of(_work, struct ib_update_work, work);
+ int ret;
+
+ /* Before distributing the cache update event, first sync
+ * the cache.
+ */
+ ret = ib_cache_update(work->event.device, work->event.element.port_num,
+ work->enforce_security);
+
+ /* GID event is notified already for individual GID entries by
+ * dispatch_gid_change_event(). Hence, notifiy for rest of the
+ * events.
+ */
+ if (!ret && work->event.event != IB_EVENT_GID_CHANGE)
+ ib_dispatch_event_clients(&work->event);
+
+ kfree(work);
}
-static void ib_cache_task(struct work_struct *_work)
+static void ib_generic_event_task(struct work_struct *_work)
{
struct ib_update_work *work =
container_of(_work, struct ib_update_work, work);
- ib_cache_update(work->device,
- work->port_num,
- work->enforce_security);
+ ib_dispatch_event_clients(&work->event);
kfree(work);
}
-static void ib_cache_event(struct ib_event_handler *handler,
- struct ib_event *event)
+static bool is_cache_update_event(const struct ib_event *event)
+{
+ return (event->event == IB_EVENT_PORT_ERR ||
+ event->event == IB_EVENT_PORT_ACTIVE ||
+ event->event == IB_EVENT_LID_CHANGE ||
+ event->event == IB_EVENT_PKEY_CHANGE ||
+ event->event == IB_EVENT_CLIENT_REREGISTER ||
+ event->event == IB_EVENT_GID_CHANGE);
+}
+
+/**
+ * ib_dispatch_event - Dispatch an asynchronous event
+ * @event:Event to dispatch
+ *
+ * Low-level drivers must call ib_dispatch_event() to dispatch the
+ * event to all registered event handlers when an asynchronous event
+ * occurs.
+ */
+void ib_dispatch_event(const struct ib_event *event)
{
struct ib_update_work *work;
- if (event->event == IB_EVENT_PORT_ERR ||
- event->event == IB_EVENT_PORT_ACTIVE ||
- event->event == IB_EVENT_LID_CHANGE ||
- event->event == IB_EVENT_PKEY_CHANGE ||
- event->event == IB_EVENT_CLIENT_REREGISTER ||
- event->event == IB_EVENT_GID_CHANGE) {
- work = kmalloc(sizeof *work, GFP_ATOMIC);
- if (work) {
- INIT_WORK(&work->work, ib_cache_task);
- work->device = event->device;
- work->port_num = event->element.port_num;
- if (event->event == IB_EVENT_PKEY_CHANGE ||
- event->event == IB_EVENT_GID_CHANGE)
- work->enforce_security = true;
- else
- work->enforce_security = false;
-
- queue_work(ib_wq, &work->work);
- }
- }
+ work = kzalloc(sizeof(*work), GFP_ATOMIC);
+ if (!work)
+ return;
+
+ if (is_cache_update_event(event))
+ INIT_WORK(&work->work, ib_cache_event_task);
+ else
+ INIT_WORK(&work->work, ib_generic_event_task);
+
+ work->event = *event;
+ if (event->event == IB_EVENT_PKEY_CHANGE ||
+ event->event == IB_EVENT_GID_CHANGE)
+ work->enforce_security = true;
+
+ queue_work(ib_wq, &work->work);
}
+EXPORT_SYMBOL(ib_dispatch_event);
int ib_cache_setup_one(struct ib_device *device)
{
@@ -1511,9 +1545,6 @@ int ib_cache_setup_one(struct ib_device *device)
rdma_for_each_port (device, p)
ib_cache_update(device, p, true);
- INIT_IB_EVENT_HANDLER(&device->cache.event_handler,
- device, ib_cache_event);
- ib_register_event_handler(&device->cache.event_handler);
return 0;
}
@@ -1535,14 +1566,12 @@ void ib_cache_release_one(struct ib_device *device)
void ib_cache_cleanup_one(struct ib_device *device)
{
- /* The cleanup function unregisters the event handler,
- * waits for all in-progress workqueue elements and cleans
- * up the GID cache. This function should be called after
- * the device was removed from the devices list and all
- * clients were removed, so the cache exists but is
+ /* The cleanup function waits for all in-progress workqueue
+ * elements and cleans up the GID cache. This function should be
+ * called after the device was removed from the devices list and
+ * all clients were removed, so the cache exists but is
* non-functional and shouldn't be updated anymore.
*/
- ib_unregister_event_handler(&device->cache.event_handler);
flush_workqueue(ib_wq);
gid_table_cleanup_one(device);
diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index 3a8b0911c3bc..362b0231f7a1 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -149,6 +149,7 @@ unsigned long roce_gid_type_mask_support(struct ib_device *ib_dev, u8 port);
int ib_cache_setup_one(struct ib_device *device);
void ib_cache_cleanup_one(struct ib_device *device);
void ib_cache_release_one(struct ib_device *device);
+void ib_dispatch_event_clients(struct ib_event *event);
#ifdef CONFIG_CGROUP_RDMA
void ib_device_register_rdmacg(struct ib_device *device);
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index a667636f74bf..3524fa73a3ea 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1951,15 +1951,7 @@ void ib_unregister_event_handler(struct ib_event_handler *event_handler)
}
EXPORT_SYMBOL(ib_unregister_event_handler);
-/**
- * ib_dispatch_event - Dispatch an asynchronous event
- * @event:Event to dispatch
- *
- * Low-level drivers must call ib_dispatch_event() to dispatch the
- * event to all registered event handlers when an asynchronous event
- * occurs.
- */
-void ib_dispatch_event(struct ib_event *event)
+void ib_dispatch_event_clients(struct ib_event *event)
{
unsigned long flags;
struct ib_event_handler *handler;
@@ -1971,7 +1963,6 @@ void ib_dispatch_event(struct ib_event *event)
spin_unlock_irqrestore(&event->device->event_handler_lock, flags);
}
-EXPORT_SYMBOL(ib_dispatch_event);
static int iw_query_port(struct ib_device *device,
u8 port_num,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6a47ba85c54c..586da67ff0f5 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2146,7 +2146,6 @@ struct ib_port_cache {
struct ib_cache {
rwlock_t lock;
- struct ib_event_handler event_handler;
};
struct ib_port_immutable {
@@ -2897,7 +2896,7 @@ bool ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
void ib_register_event_handler(struct ib_event_handler *event_handler);
void ib_unregister_event_handler(struct ib_event_handler *event_handler);
-void ib_dispatch_event(struct ib_event *event);
+void ib_dispatch_event(const struct ib_event *event);
int ib_query_port(struct ib_device *device,
u8 port_num, struct ib_port_attr *port_attr);
--
2.19.2
