| Message ID | alpine.LRH.2.02.1711222134370.2330@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
Please run this past the swait authors. It is supposed to be a simple and self-contained API so I'd expect this patch to be seen critical. You might be better off to just use the normal complex waitqueues if you want to micro-optimize like this. On Wed, Nov 22, 2017 at 09:35:36PM -0500, Mikulas Patocka wrote: > In order to reduce locking overhead, I use the spinlock in > swait_queue_head to protect not only the wait queue, but also the list of > events. Consequently, I need to use unlocked functions __prepare_to_swait > and __finish_swait. These functions are declared in the file > include/linux/swait.h, but they are not exported, and so they are not > useable from kernel modules. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > --- > kernel/sched/swait.c | 2 ++ > 1 file changed, 2 insertions(+) > > Index: linux-2.6/kernel/sched/swait.c > =================================================================== > --- linux-2.6.orig/kernel/sched/swait.c > +++ linux-2.6/kernel/sched/swait.c > @@ -73,6 +73,7 @@ void __prepare_to_swait(struct swait_que > if (list_empty(&wait->task_list)) > list_add(&wait->task_list, &q->task_list); > } > +EXPORT_SYMBOL(__prepare_to_swait); > > void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state) > { > @@ -102,6 +103,7 @@ void __finish_swait(struct swait_queue_h > if (!list_empty(&wait->task_list)) > list_del_init(&wait->task_list); > } > +EXPORT_SYMBOL(__finish_swait); > > void finish_swait(struct swait_queue_head *q, struct swait_queue *wait) > { > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel ---end quoted text--- -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 23 Nov 2017, Christoph Hellwig wrote: > Please run this past the swait authors. It is supposed to be a simple > and self-contained API so I'd expect this patch to be seen critical. I already sent it to Peter Zijlstra and didn't get a response yet. > You might be better off to just use the normal complex waitqueues if > you want to micro-optimize like this. If we wanted to micro-optimize, we should use the simpler wait queue variant. If these functions are not supposed to be used by others, then - why are they in in file swait.h? - why does the implementation export swake_up_locked which assumes that someone else will lock the spinlock before calling it? > On Wed, Nov 22, 2017 at 09:35:36PM -0500, Mikulas Patocka wrote: > > In order to reduce locking overhead, I use the spinlock in > > swait_queue_head to protect not only the wait queue, but also the list of > > events. Consequently, I need to use unlocked functions __prepare_to_swait > > and __finish_swait. These functions are declared in the file > > include/linux/swait.h, but they are not exported, and so they are not > > useable from kernel modules. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > > --- > > kernel/sched/swait.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > Index: linux-2.6/kernel/sched/swait.c > > =================================================================== > > --- linux-2.6.orig/kernel/sched/swait.c > > +++ linux-2.6/kernel/sched/swait.c > > @@ -73,6 +73,7 @@ void __prepare_to_swait(struct swait_que > > if (list_empty(&wait->task_list)) > > list_add(&wait->task_list, &q->task_list); > > } > > +EXPORT_SYMBOL(__prepare_to_swait); > > > > void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state) > > { > > @@ -102,6 +103,7 @@ void __finish_swait(struct swait_queue_h > > if (!list_empty(&wait->task_list)) > > list_del_init(&wait->task_list); > > } > > +EXPORT_SYMBOL(__finish_swait); > > > > void finish_swait(struct swait_queue_head *q, struct swait_queue *wait) > > { > > > > -- > > dm-devel mailing list > > dm-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/dm-devel > ---end quoted text--- > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Nov 23 2017 at 5:27pm -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Thu, 23 Nov 2017, Christoph Hellwig wrote: > > > Please run this past the swait authors. It is supposed to be a simple > > and self-contained API so I'd expect this patch to be seen critical. > > I already sent it to Peter Zijlstra and didn't get a response yet. > > > You might be better off to just use the normal complex waitqueues if > > you want to micro-optimize like this. > > If we wanted to micro-optimize, we should use the simpler wait queue > variant. > > > If these functions are not supposed to be used by others, then > - why are they in in file swait.h? > - why does the implementation export swake_up_locked which assumes that > someone else will lock the spinlock before calling it? Hi, I'd like to get this patch upstream. I'm happy to send it to Linus via linux-dm.git but I wanted to check with others who might care more deeply about swait interfaces to get their Ack (or otherwise): https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.18&id=4a2ec3f321f83db09da4824025420586c9ef1612 Here is Mikulas' DM driver code that makes use of __prepare_to_swait() and __finish_swait(): static int writecache_endio_thread(void *data) { struct dm_writecache *wc = data; while (1) { DECLARE_SWAITQUEUE(wait); struct list_head list; raw_spin_lock_irq(&wc->endio_thread_wait.lock); continue_locked: if (!list_empty(&wc->endio_list)) goto pop_from_list; set_current_state(TASK_INTERRUPTIBLE); __prepare_to_swait(&wc->endio_thread_wait, &wait); raw_spin_unlock_irq(&wc->endio_thread_wait.lock); if (unlikely(kthread_should_stop())) { finish_swait(&wc->endio_thread_wait, &wait); break; } schedule(); raw_spin_lock_irq(&wc->endio_thread_wait.lock); __finish_swait(&wc->endio_thread_wait, &wait); goto continue_locked; pop_from_list: list = wc->endio_list; list.next->prev = list.prev->next = &list; INIT_LIST_HEAD(&wc->endio_list); raw_spin_unlock_irq(&wc->endio_thread_wait.lock); ... -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Index: linux-2.6/kernel/sched/swait.c =================================================================== --- linux-2.6.orig/kernel/sched/swait.c +++ linux-2.6/kernel/sched/swait.c @@ -73,6 +73,7 @@ void __prepare_to_swait(struct swait_que if (list_empty(&wait->task_list)) list_add(&wait->task_list, &q->task_list); } +EXPORT_SYMBOL(__prepare_to_swait); void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state) { @@ -102,6 +103,7 @@ void __finish_swait(struct swait_queue_h if (!list_empty(&wait->task_list)) list_del_init(&wait->task_list); } +EXPORT_SYMBOL(__finish_swait); void finish_swait(struct swait_queue_head *q, struct swait_queue *wait) {
In order to reduce locking overhead, I use the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, I need to use unlocked functions __prepare_to_swait and __finish_swait. These functions are declared in the file include/linux/swait.h, but they are not exported, and so they are not useable from kernel modules. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- kernel/sched/swait.c | 2 ++ 1 file changed, 2 insertions(+) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel