Re: [PATCH v2.6.36-rc7] infiniband: update workqueue usage
Hello, On 10/20/2010 12:21 PM, Bart Van Assche wrote: >> Hmm... that's one very interesting reason to be unhappy. Can you >> please elaborate why addition of a global variable doesn't make you >> happy? > > In the past every time I saw a global variable being added in a > software project that meant that some software concept was not being > abstracted properly. Which does not necessarily mean that that is the > case with this patch. That's too wide brushed position to agree with, but, well, let's talk about that some other time. :-) > With the posted patch, just like with the current implementation, e.g. > the flush_workqueue() call in the ipath driver will make that driver > wait until work scheduled by the core/sa_query.c code finished and > vice versa. Is that necessary ? If not, using multiple work queues for > IB instead of one would allow to get rid of that global ib_wq > declaration. Yeap, if breaking it down further makes sense, definitely. If someone is interested, the followings are the guidelines I've been following in these conversions. * If all the possibly pending works can be safely enumerated without too much difficulty and the works in question are not used during memory reclaim, use the system workqueue and use explicit flush/cancel_work[_sync]() instead of using flush_workqueue(). Please note that flush_work_sync() isn't in mainline yet. It's scheduled for the coming rc1 window. * If works can be used during memory reclaim, there's no way around it. A separate workqueue needs to be used. * If works free themselves or are used to put the last reference of the containing object, they can't be flushed explicitly and thus need to be queued on a dedicated workqueue which serves as the flushing domain. For this patch, I chose less intrusive path and just replaced system wide workqueue with subsystem wide one mainly because I don't have experience with or access to anything infiniband. If someone wants to push it further, I would be happy to help. BTW, does the posted patch look safe to you? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.6.36-rc7] infiniband: update workqueue usage
On Wed, Oct 20, 2010 at 10:38 AM, Tejun Heo wrote: > Hello, > > On 10/19/2010 08:40 PM, Bart Van Assche wrote: >> On Tue, Oct 19, 2010 at 5:24 PM, Tejun Heo wrote: >>> [ ... ] >>> This is to prepare for deprecation of flush_scheduled_work(). >>> [ ... ] >>> Index: work/include/rdma/ib_verbs.h >>> [ ... ] >>> +extern struct workqueue_struct *ib_wq; >>> [ ... ] >> >> This patch adds a declaration of a global variable to a public header >> file. That might be unavoidable, but it doesn't make me happy. > > Hmm... that's one very interesting reason to be unhappy. Can you > please elaborate why addition of a global variable doesn't make you > happy? In the past every time I saw a global variable being added in a software project that meant that some software concept was not being abstracted properly. Which does not necessarily mean that that is the case with this patch. With the posted patch, just like with the current implementation, e.g. the flush_workqueue() call in the ipath driver will make that driver wait until work scheduled by the core/sa_query.c code finished and vice versa. Is that necessary ? If not, using multiple work queues for IB instead of one would allow to get rid of that global ib_wq declaration. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.6.36-rc7] infiniband: update workqueue usage
Hello, On 10/19/2010 08:40 PM, Bart Van Assche wrote: > On Tue, Oct 19, 2010 at 5:24 PM, Tejun Heo wrote: >> [ ... ] >> This is to prepare for deprecation of flush_scheduled_work(). >> [ ... ] >> Index: work/include/rdma/ib_verbs.h >> [ ... ] >> +extern struct workqueue_struct *ib_wq; >> [ ... ] > > This patch adds a declaration of a global variable to a public header > file. That might be unavoidable, but it doesn't make me happy. Hmm... that's one very interesting reason to be unhappy. Can you please elaborate why addition of a global variable doesn't make you happy? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.6.36-rc7] infiniband: update workqueue usage
Hello, On 10/19/2010 07:22 PM, Ralph Campbell wrote: > On Tue, 2010-10-19 at 08:24 -0700, Tejun Heo wrote: > >> * qib_cq_wq is a separate singlethread workqueue. Does the queue >> require strict single thread execution ordering? IOW, does each >> work have to be executed in the exact queued order and no two works >> should execute in parallel? Or was the singlethreadedness chosen >> just to reduce the number of workers? > > The work functions need to be called in-order and single threaded > or memory will be freed multiple times and other "bad things". I see, so they'll need to be converted to alloc_ordered_workqueue() once -rc1 merge window opens up. I'll follow up with the conversion. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.6.36-rc7] infiniband: update workqueue usage
On Tue, Oct 19, 2010 at 5:24 PM, Tejun Heo wrote: > [ ... ] > This is to prepare for deprecation of flush_scheduled_work(). > [ ... ] > Index: work/include/rdma/ib_verbs.h > [ ... ] > +extern struct workqueue_struct *ib_wq; > [ ... ] This patch adds a declaration of a global variable to a public header file. That might be unavoidable, but it doesn't make me happy. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.6.36-rc7] infiniband: update workqueue usage
On Tue, 2010-10-19 at 08:24 -0700, Tejun Heo wrote: > * qib_cq_wq is a separate singlethread workqueue. Does the queue > require strict single thread execution ordering? IOW, does each > work have to be executed in the exact queued order and no two works > should execute in parallel? Or was the singlethreadedness chosen > just to reduce the number of workers? The work functions need to be called in-order and single threaded or memory will be freed multiple times and other "bad things". -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2.6.36-rc7] infiniband: update workqueue usage
* ib_wq is added, which is used as the common workqueue for infiniband
instead of the system workqueue. All system workqueue usages
including flush_scheduled_work() callers are converted to use and
flush ib_wq.
* cancel_delayed_work() + flush_scheduled_work() converted to
cancel_delayed_work_sync().
* qib_wq is removed and ib_wq is used instead.
This is to prepare for deprecation of flush_scheduled_work().
Signed-off-by: Tejun Heo
---
Hello,
I think this patch is safe but don't have any experience with or
access to infiniband stuff and it's only compile tested. Also, while
looking through the code, I got curious about several things.
* Can any of the works in infiniband be used during memory reclaim?
* qib_cq_wq is a separate singlethread workqueue. Does the queue
require strict single thread execution ordering? IOW, does each
work have to be executed in the exact queued order and no two works
should execute in parallel? Or was the singlethreadedness chosen
just to reduce the number of workers?
* The same question for ipoib_workqueue.
Thank you.
drivers/infiniband/core/cache.c|4 +--
drivers/infiniband/core/device.c | 11 --
drivers/infiniband/core/sa_query.c |4 +--
drivers/infiniband/core/umem.c |2 -
drivers/infiniband/hw/ipath/ipath_driver.c |2 -
drivers/infiniband/hw/ipath/ipath_user_pages.c |2 -
drivers/infiniband/hw/qib/qib_iba7220.c|7 ++
drivers/infiniband/hw/qib/qib_iba7322.c| 14 ++---
drivers/infiniband/hw/qib/qib_init.c | 26 +++--
drivers/infiniband/hw/qib/qib_qsfp.c |9 +++-
drivers/infiniband/hw/qib/qib_verbs.h |5 +---
drivers/infiniband/ulp/srp/ib_srp.c|4 +--
include/rdma/ib_verbs.h|3 ++
13 files changed, 41 insertions(+), 52 deletions(-)
Index: work/drivers/infiniband/core/cache.c
===
--- work.orig/drivers/infiniband/core/cache.c
+++ work/drivers/infiniband/core/cache.c
@@ -308,7 +308,7 @@ static void ib_cache_event(struct ib_eve
INIT_WORK(&work->work, ib_cache_task);
work->device = event->device;
work->port_num = event->element.port_num;
- schedule_work(&work->work);
+ queue_work(ib_wq, &work->work);
}
}
}
@@ -368,7 +368,7 @@ static void ib_cache_cleanup_one(struct
int p;
ib_unregister_event_handler(&device->cache.event_handler);
- flush_scheduled_work();
+ flush_workqueue(ib_wq);
for (p = 0; p <= end_port(device) - start_port(device); ++p) {
kfree(device->cache.pkey_cache[p]);
Index: work/drivers/infiniband/core/device.c
===
--- work.orig/drivers/infiniband/core/device.c
+++ work/drivers/infiniband/core/device.c
@@ -38,7 +38,6 @@
#include
#include
#include
-#include
#include "core_priv.h"
@@ -52,6 +51,9 @@ struct ib_client_data {
void *data;
};
+struct workqueue_struct *ib_wq;
+EXPORT_SYMBOL_GPL(ib_wq);
+
static LIST_HEAD(device_list);
static LIST_HEAD(client_list);
@@ -718,6 +720,10 @@ static int __init ib_core_init(void)
{
int ret;
+ ib_wq = alloc_workqueue("infiniband", 0, 0);
+ if (!ib_wq)
+ return -ENOMEM;
+
ret = ib_sysfs_setup();
if (ret)
printk(KERN_WARNING "Couldn't create InfiniBand device
class\n");
@@ -726,6 +732,7 @@ static int __init ib_core_init(void)
if (ret) {
printk(KERN_WARNING "Couldn't set up InfiniBand P_Key/GID
cache\n");
ib_sysfs_cleanup();
+ destroy_workqueue(ib_wq);
}
return ret;
@@ -736,7 +743,7 @@ static void __exit ib_core_cleanup(void)
ib_cache_cleanup();
ib_sysfs_cleanup();
/* Make sure that any pending umem accounting work is done. */
- flush_scheduled_work();
+ destroy_workqueue(ib_wq);
}
module_init(ib_core_init);
Index: work/drivers/infiniband/core/sa_query.c
===
--- work.orig/drivers/infiniband/core/sa_query.c
+++ work/drivers/infiniband/core/sa_query.c
@@ -422,7 +422,7 @@ static void ib_sa_event(struct ib_event_
port->sm_ah = NULL;
spin_unlock_irqrestore(&port->ah_lock, flags);
- schedule_work(&sa_dev->port[event->element.port_num -
+ queue_work(ib_wq, &sa_dev->port[event->element.port_num -
sa_dev->start_port].update_task);
}
}
@@ -1068,7 +1068,7 @@ static void ib_sa_remove_one(struct ib_d
ib_unregister_event_handler(&sa_dev->event_handler);
- flush_scheduled_wor
