Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255415 > unrolled thread

[PATCH -mm 0/3] nbd: fix/cleanup the usage of kernel_dequeue_signal()

Started byOleg Nesterov <oleg@redhat.com>
First post2015-10-25 15:40 +0100
Last post2015-10-26 08:50 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH -mm 0/3] nbd: fix/cleanup the usage of  kernel_dequeue_signal() Oleg Nesterov <oleg@redhat.com> - 2015-10-25 15:40 +0100
    [PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking  nbd->tasks_lock Oleg Nesterov <oleg@redhat.com> - 2015-10-25 15:40 +0100
      Re: [PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking  nbd->tasks_lock Markus Pargmann <mpa@pengutronix.de> - 2015-10-26 09:00 +0100
    [PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary  kernel_dequeue_signal() Oleg Nesterov <oleg@redhat.com> - 2015-10-25 15:40 +0100
      Re: [PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary  kernel_dequeue_signal() Markus Pargmann <mpa@pengutronix.de> - 2015-10-26 08:50 +0100

#1255415 — [PATCH -mm 0/3] nbd: fix/cleanup the usage of kernel_dequeue_signal()

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-25 15:40 +0100
Subject[PATCH -mm 0/3] nbd: fix/cleanup the usage of kernel_dequeue_signal()
Message-ID<qnuCK-481-3@gated-at.bofh.it>
On top of

	signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal
	
in -mm plus

	signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal-fix

I sent today.

Another untested (but simple) series. Needs an ack from  Markus or
should be ignored, I can easily miss something.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1255417 — [PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking nbd->tasks_lock

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-25 15:40 +0100
Subject[PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking nbd->tasks_lock
Message-ID<qnuCK-481-17@gated-at.bofh.it>
In reply to#1255415
Both nbd_thread_recv() and nbd_thread_send() are might_sleep() and
called with irqs enabled(), irqsave/irqrestore make no sense and imo
look confusing.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 drivers/block/nbd.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0ffd73c..fd79405 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -406,23 +406,22 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 {
 	struct request *req;
 	int ret;
-	unsigned long flags;
 
 	BUG_ON(nbd->magic != NBD_MAGIC);
 
 	sk_set_memalloc(nbd->sock->sk);
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_recv = current;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
 	if (ret) {
 		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
 
-		spin_lock_irqsave(&nbd->tasks_lock, flags);
+		spin_lock_irq(&nbd->tasks_lock);
 		nbd->task_recv = NULL;
-		spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+		spin_unlock_irq(&nbd->tasks_lock);
 
 		return ret;
 	}
@@ -439,9 +438,9 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_recv = NULL;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	if (signal_pending(current)) {
 		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal",
@@ -543,11 +542,10 @@ static int nbd_thread_send(void *data)
 {
 	struct nbd_device *nbd = data;
 	struct request *req;
-	unsigned long flags;
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_send = current;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	set_user_nice(current, MIN_NICE);
 	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
@@ -581,9 +579,9 @@ static int nbd_thread_send(void *data)
 		nbd_handle_req(nbd, req);
 	}
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_send = NULL;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	return 0;
 }
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255719 — Re: [PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking nbd->tasks_lock

FromMarkus Pargmann <mpa@pengutronix.de>
Date2015-10-26 09:00 +0100
SubjectRe: [PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking nbd->tasks_lock
Message-ID<qnKRc-5uZ-11@gated-at.bofh.it>
In reply to#1255417

[Multipart message — attachments visible in raw view] — view raw

On Sun, Oct 25, 2015 at 04:26:41PM +0100, Oleg Nesterov wrote:
> Both nbd_thread_recv() and nbd_thread_send() are might_sleep() and
> called with irqs enabled(), irqsave/irqrestore make no sense and imo
> look confusing.

Thanks, applied.

Regards,

Markus

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  drivers/block/nbd.c |   22 ++++++++++------------
>  1 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0ffd73c..fd79405 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -406,23 +406,22 @@ static int nbd_thread_recv(struct nbd_device *nbd)
>  {
>  	struct request *req;
>  	int ret;
> -	unsigned long flags;
>  
>  	BUG_ON(nbd->magic != NBD_MAGIC);
>  
>  	sk_set_memalloc(nbd->sock->sk);
>  
> -	spin_lock_irqsave(&nbd->tasks_lock, flags);
> +	spin_lock_irq(&nbd->tasks_lock);
>  	nbd->task_recv = current;
> -	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +	spin_unlock_irq(&nbd->tasks_lock);
>  
>  	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
>  	if (ret) {
>  		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
>  
> -		spin_lock_irqsave(&nbd->tasks_lock, flags);
> +		spin_lock_irq(&nbd->tasks_lock);
>  		nbd->task_recv = NULL;
> -		spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +		spin_unlock_irq(&nbd->tasks_lock);
>  
>  		return ret;
>  	}
> @@ -439,9 +438,9 @@ static int nbd_thread_recv(struct nbd_device *nbd)
>  
>  	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
>  
> -	spin_lock_irqsave(&nbd->tasks_lock, flags);
> +	spin_lock_irq(&nbd->tasks_lock);
>  	nbd->task_recv = NULL;
> -	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +	spin_unlock_irq(&nbd->tasks_lock);
>  
>  	if (signal_pending(current)) {
>  		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal",
> @@ -543,11 +542,10 @@ static int nbd_thread_send(void *data)
>  {
>  	struct nbd_device *nbd = data;
>  	struct request *req;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&nbd->tasks_lock, flags);
> +	spin_lock_irq(&nbd->tasks_lock);
>  	nbd->task_send = current;
> -	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +	spin_unlock_irq(&nbd->tasks_lock);
>  
>  	set_user_nice(current, MIN_NICE);
>  	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
> @@ -581,9 +579,9 @@ static int nbd_thread_send(void *data)
>  		nbd_handle_req(nbd, req);
>  	}
>  
> -	spin_lock_irqsave(&nbd->tasks_lock, flags);
> +	spin_lock_irq(&nbd->tasks_lock);
>  	nbd->task_send = NULL;
> -	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
> +	spin_unlock_irq(&nbd->tasks_lock);
>  
>  	return 0;
>  }
> -- 
> 1.5.5.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[toc] | [prev] | [next] | [standalone]

#1255418 — [PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary kernel_dequeue_signal()

FromOleg Nesterov <oleg@redhat.com>
Date2015-10-25 15:40 +0100
Subject[PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary kernel_dequeue_signal()
Message-ID<qnuCK-481-21@gated-at.bofh.it>
In reply to#1255415
nbd_thread_send() does kernel_dequeue_signal() at the end for no reason,
it is fine to exit with the pending SIGKILL.

Not sure it really needs another kernel_dequeue_signal() inside the
main loop, we know that signal_pending() means SIGKILL. But probably
we want to clear TIF_SIGPENDING before sock_shutdown().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 drivers/block/nbd.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index b85e7a0..e5d96e5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -586,10 +586,6 @@ static int nbd_thread_send(void *data)
 	nbd->task_send = NULL;
 	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
-	/* Clear maybe pending signals */
-	if (signal_pending(current))
-		kernel_dequeue_signal(NULL);
-
 	return 0;
 }
 
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255715 — Re: [PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary kernel_dequeue_signal()

FromMarkus Pargmann <mpa@pengutronix.de>
Date2015-10-26 08:50 +0100
SubjectRe: [PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary kernel_dequeue_signal()
Message-ID<qnKHv-5rJ-3@gated-at.bofh.it>
In reply to#1255418

[Multipart message — attachments visible in raw view] — view raw

On Sun, Oct 25, 2015 at 04:26:37PM +0100, Oleg Nesterov wrote:
> nbd_thread_send() does kernel_dequeue_signal() at the end for no reason,
> it is fine to exit with the pending SIGKILL.
> 
> Not sure it really needs another kernel_dequeue_signal() inside the
> main loop, we know that signal_pending() means SIGKILL. But probably
> we want to clear TIF_SIGPENDING before sock_shutdown().

Yes, I don't think it would be good to go into a tcp socket shutdown
with a pending signal although I don't know if this would make
difficulties.

I will apply the patch to my tree and send it out as pull request to
Jens as usual.

Thanks,

Markus

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  drivers/block/nbd.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index b85e7a0..e5d96e5 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -586,10 +586,6 @@ static int nbd_thread_send(void *data)
>  	nbd->task_send = NULL;
>  	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
>  
> -	/* Clear maybe pending signals */
> -	if (signal_pending(current))
> -		kernel_dequeue_signal(NULL);
> -
>  	return 0;
>  }
>  
> -- 
> 1.5.5.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web