[PATCH 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[PATCH 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[Mikulas Patocka]

[-- Attachment #1: export-__finish_swait-__prepare_to_swait.patch --]
[-- Type: text/plain, Size: 1265 bytes --]

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	2018-04-16 21:10:05.000000000 +0200
+++ linux-2.6/kernel/sched/swait.c	2018-04-16 21:10:05.000000000 +0200
@@ -75,6 +75,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)
 {
@@ -104,6 +105,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)
 {

[patch 0/4] dm-writecache patches

[Mikulas Patocka]

Hi

Here I'm sending the dm-writecache patches.

The first patch optimizes x86 memcpy_flushcache for small constant size.
It increases dm-writecache throughput by about 2%. It should be already in
Dan Williams' tree.

The second patch exports __prepare_to_swait and __finish_swait.

The third patch is dm-writecache that is already in Mike's tree.

The fourth patch converts it to use the new API. The pmem_* API is at the
beginning of the file dm-writecache.c - it may be moved to system include
files.

Mikulas

[patch 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[Mikulas Patocka]

[-- Attachment #1: export-__finish_swait-__prepare_to_swait.patch --]
[-- Type: text/plain, Size: 1265 bytes --]

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	2018-04-16 21:10:05.000000000 +0200
+++ linux-2.6/kernel/sched/swait.c	2018-04-16 21:10:05.000000000 +0200
@@ -75,6 +75,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)
 {
@@ -104,6 +105,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)
 {

Re: [patch 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[Christoph Hellwig]

On Sat, May 19, 2018 at 07:25:05AM +0200, 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.

Please CC the author and maintainers of the swait code.

My impression is that this is the wrong thing to do.  The swait code
is supposed to be simple and self contained, and if you want to do
anything else use normal waitqueues.

> 
> 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	2018-04-16 21:10:05.000000000 +0200
> +++ linux-2.6/kernel/sched/swait.c	2018-04-16 21:10:05.000000000 +0200
> @@ -75,6 +75,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)
>  {
> @@ -104,6 +105,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---

Re: [patch 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[Mike Snitzer]

On Tue, May 22 2018 at  2:34am -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> On Sat, May 19, 2018 at 07:25:05AM +0200, 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.
> 
> Please CC the author and maintainers of the swait code.
> 
> My impression is that this is the wrong thing to do.  The swait code
> is supposed to be simple and self contained, and if you want to do
> anything else use normal waitqueues.

You said the same thing last time around.  I've since cc'd Peter and
Thomas and haven't heard back, see:
https://www.redhat.com/archives/dm-devel/2018-May/msg00048.html

The entire point of exporting these symbols is to allow use of the
"simple waitqueue" code to optimize -- without resorting to using normal
waitqueues.

Mike

Re: [patch 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[Peter Zijlstra]

On Tue, May 22, 2018 at 02:52:54PM -0400, Mike Snitzer wrote:
> On Tue, May 22 2018 at  2:34am -0400,
> Christoph Hellwig <hch@infradead.org> wrote:

> > Please CC the author and maintainers of the swait code.
> > 
> > My impression is that this is the wrong thing to do.  The swait code
> > is supposed to be simple and self contained, and if you want to do
> > anything else use normal waitqueues.
> 
> You said the same thing last time around.  I've since cc'd Peter and
> Thomas and haven't heard back, see:
> https://www.redhat.com/archives/dm-devel/2018-May/msg00048.html

Yeah, sorry, got lost :/

> The entire point of exporting these symbols is to allow use of the
> "simple waitqueue" code to optimize -- without resorting to using normal
> waitqueues.

So I don't immediately object to exporting them; however I do share some
of hch's concerns. The reason swait exists is to be deterministic (for
RT) -- something that regular wait code cannot be.

And by (ab)using / exporting the wait internal lock you risk loosing
that. So I don't think the proposed usage is bad, it is possible to
create badness.

So if we're going to export them; someone needs to keep an eye on things
and ensure the lock isn't abused.

Re: [patch 2/4] swait: export the symbols __prepare_to_swait and __finish_swait

[Mike Snitzer]

On Wed, May 23 2018 at  5:21am -0400,
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, May 22, 2018 at 02:52:54PM -0400, Mike Snitzer wrote:
> > On Tue, May 22 2018 at  2:34am -0400,
> > Christoph Hellwig <hch@infradead.org> wrote:
> 
> > > Please CC the author and maintainers of the swait code.
> > > 
> > > My impression is that this is the wrong thing to do.  The swait code
> > > is supposed to be simple and self contained, and if you want to do
> > > anything else use normal waitqueues.
> > 
> > You said the same thing last time around.  I've since cc'd Peter and
> > Thomas and haven't heard back, see:
> > https://www.redhat.com/archives/dm-devel/2018-May/msg00048.html
> 
> Yeah, sorry, got lost :/

np

> > The entire point of exporting these symbols is to allow use of the
> > "simple waitqueue" code to optimize -- without resorting to using normal
> > waitqueues.
> 
> So I don't immediately object to exporting them; however I do share some
> of hch's concerns. The reason swait exists is to be deterministic (for
> RT) -- something that regular wait code cannot be.
> 
> And by (ab)using / exporting the wait internal lock you risk loosing
> that. So I don't think the proposed usage is bad, it is possible to
> create badness.

Understood.

> So if we're going to export them; someone needs to keep an eye on things
> and ensure the lock isn't abused.

I'll update the patch header and swait.h to reflect these requirements
and send out a new patch.

If you could then reply with your explicit Ack I can stage it for 4.18
via linux-dm.git to ease cross tree dependencies (given dm-writecache
depends on these exports) -- provided you're OK with me doing that.

Thanks,
Mike