public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Frederic Weisbecker <fweisbec@gmail.com>
To: LKML <linux-kernel@vger.kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@infradead.org>,
	Ingo Molnar <mingo@kernel.org>, Jan Kara <jack@suse.cz>,
	Jens Axboe <jens.axboe@oracle.com>
Subject: [PATCH 10/11] smp: Remove wait argument from __smp_call_function_single()
Date: Sat,  8 Feb 2014 17:18:39 +0100	[thread overview]
Message-ID: <1391876320-25068-11-git-send-email-fweisbec@gmail.com> (raw)
In-Reply-To: <1391876320-25068-1-git-send-email-fweisbec@gmail.com>

The main point of calling __smp_call_function_single() is to send
an IPI in a pure asynchronous way. By embedding a csd in an object,
a caller can send the IPI without waiting for a previous one to complete
as is required by smp_call_function_single() for example. As such,
sending this kind of IPI can be safe even when irqs are disabled.

This flexibility comes at the expense of the caller who then needs to
synchronize the csd lifecycle by himself and make sure that IPIs on a
single csd are serialized.

This is how __smp_call_function_single() works when wait = 0 and this
usecase is relevant.

Now there don't seem to be any usecase with wait = 1 that can't be
covered by smp_call_function_single() instead, which is safer. Lets look
at the two possible scenario:

1) The user calls __smp_call_function_single(wait = 1) on a csd embedded
   in an object. It looks like a nice and convenient pattern at the first
   sight because we can then retrieve the object from the IPI handler easily.

   But actually it is a waste of memory space in the object since the csd
   can be allocated from the stack by smp_call_function_single(wait = 1)
   and the object can be passed an the IPI argument.

   Besides that, embedding the csd in an object is more error prone
   because the caller must take care of the serialization of the IPIs
   for this csd.

2) The user calls __smp_call_function_single(wait = 1) on a csd that
   is allocated on the stack. It's ok but smp_call_function_single()
   can do it as well and it already takes care of the allocation on the
   stack. Again it's more simple and less error prone.

Therefore, using the underscore prepend API version with wait = 1
is a bad pattern and a sign that the caller can do safer and more
simple.

There was a single user of that which has just been converted.
So lets remove this option to discourage further users.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Jens Axboe <jens.axboe@oracle.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 block/blk-mq.c            |  2 +-
 block/blk-softirq.c       |  2 +-
 drivers/block/null_blk.c  |  2 +-
 drivers/cpuidle/coupled.c |  2 +-
 include/linux/smp.h       |  2 +-
 kernel/sched/core.c       |  2 +-
 kernel/smp.c              | 19 ++++---------------
 kernel/up.c               |  3 +--
 net/core/dev.c            |  2 +-
 9 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 57039fc..87539d3 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -363,7 +363,7 @@ void blk_mq_end_io(struct request *rq, int error)
 		rq->csd.func = blk_mq_end_io_remote;
 		rq->csd.info = rq;
 		rq->csd.flags = 0;
-		__smp_call_function_single(ctx->cpu, &rq->csd, 0);
+		__smp_call_function_single(ctx->cpu, &rq->csd);
 	} else {
 		__blk_mq_end_io(rq, error);
 	}
diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index b5c37d9..6345b7e 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -70,7 +70,7 @@ static int raise_blk_irq(int cpu, struct request *rq)
 		data->info = rq;
 		data->flags = 0;
 
-		__smp_call_function_single(cpu, data, 0);
+		__smp_call_function_single(cpu, data);
 		return 0;
 	}
 
diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index 3107282..c6722dd 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -255,7 +255,7 @@ static void null_cmd_end_ipi(struct nullb_cmd *cmd)
 	if (llist_add(&cmd->ll_list, &cq->list)) {
 		data->func = null_ipi_cmd_end_io;
 		data->flags = 0;
-		__smp_call_function_single(cpu, data, 0);
+		__smp_call_function_single(cpu, data);
 	}
 
 	put_cpu();
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index e952936..0411594 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -323,7 +323,7 @@ static void cpuidle_coupled_poke(int cpu)
 	struct call_single_data *csd = &per_cpu(cpuidle_coupled_poke_cb, cpu);
 
 	if (!cpumask_test_and_set_cpu(cpu, &cpuidle_coupled_poke_pending))
-		__smp_call_function_single(cpu, csd, 0);
+		__smp_call_function_single(cpu, csd);
 }
 
 /**
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 1e8c721..3664ef3 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -50,7 +50,7 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info),
 		smp_call_func_t func, void *info, bool wait,
 		gfp_t gfp_flags);
 
-int __smp_call_function_single(int cpu, struct call_single_data *csd, int wait);
+int __smp_call_function_single(int cpu, struct call_single_data *csd);
 
 #ifdef CONFIG_SMP
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b46131e..eba3d84 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -432,7 +432,7 @@ void hrtick_start(struct rq *rq, u64 delay)
 	if (rq == this_rq()) {
 		__hrtick_restart(rq);
 	} else if (!rq->hrtick_csd_pending) {
-		__smp_call_function_single(cpu_of(rq), &rq->hrtick_csd, 0);
+		__smp_call_function_single(cpu_of(rq), &rq->hrtick_csd);
 		rq->hrtick_csd_pending = 1;
 	}
 }
diff --git a/kernel/smp.c b/kernel/smp.c
index fa04ab9..b767631 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -241,29 +241,18 @@ EXPORT_SYMBOL(smp_call_function_single);
  * __smp_call_function_single(): Run a function on a specific CPU
  * @cpu: The CPU to run on.
  * @csd: Pre-allocated and setup data structure
- * @wait: If true, wait until function has completed on specified CPU.
  *
  * Like smp_call_function_single(), but allow caller to pass in a
  * pre-allocated data structure. Useful for embedding @data inside
  * other structures, for instance.
  */
-int __smp_call_function_single(int cpu, struct call_single_data *csd, int wait)
+int __smp_call_function_single(int cpu, struct call_single_data *csd)
 {
 	int err = 0;
-	int this_cpu;
 
-	this_cpu = get_cpu();
-	/*
-	 * Can deadlock when called with interrupts disabled.
-	 * We allow cpu's that are not yet online though, as no one else can
-	 * send smp call function interrupt to this cpu and as such deadlocks
-	 * can't happen.
-	 */
-	WARN_ON_ONCE(cpu_online(this_cpu) && wait && irqs_disabled()
-		     && !oops_in_progress);
-
-	err = generic_exec_single(cpu, csd, csd->func, csd->info, wait);
-	put_cpu();
+	preempt_disable();
+	err = generic_exec_single(cpu, csd, csd->func, csd->info, 0);
+	preempt_enable();
 
 	return err;
 }
diff --git a/kernel/up.c b/kernel/up.c
index cdf03d1..4e199d4 100644
--- a/kernel/up.c
+++ b/kernel/up.c
@@ -22,8 +22,7 @@ int smp_call_function_single(int cpu, void (*func) (void *info), void *info,
 }
 EXPORT_SYMBOL(smp_call_function_single);
 
-int __smp_call_function_single(int cpu, struct call_single_data *csd,
-			       int wait)
+int __smp_call_function_single(int cpu, struct call_single_data *csd)
 {
 	unsigned long flags;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 3721db7..3f659b1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4129,7 +4129,7 @@ static void net_rps_action_and_irq_enable(struct softnet_data *sd)
 
 			if (cpu_online(remsd->cpu))
 				__smp_call_function_single(remsd->cpu,
-							   &remsd->csd, 0);
+							   &remsd->csd);
 			remsd = next;
 		}
 	} else
-- 
1.8.3.1


  parent reply	other threads:[~2014-02-08 16:19 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-02-08 16:18 [RFC PATCH 00/11] smp: Single IPI cleanups Frederic Weisbecker
2014-02-08 16:18 ` [PATCH 01/11] block: Stop abusing csd.list for fifo_time Frederic Weisbecker
2014-02-08 16:18 ` [PATCH 02/11] block: Remove useless IPI struct initialization Frederic Weisbecker
2014-02-10 20:21   ` Jan Kara
2014-02-08 16:18 ` [PATCH 03/11] block: Stop abusing rq->csd.list in blk-softirq Frederic Weisbecker
2014-02-08 16:18 ` [PATCH 04/11] smp: Iterate functions through llist_for_each_entry_safe() Frederic Weisbecker
2014-02-08 16:18 ` [PATCH 05/11] smp: Remove unused list_head from csd Frederic Weisbecker
2014-02-08 16:18 ` [PATCH 06/11] smp: Teach __smp_call_function_single() to check for offline cpus Frederic Weisbecker
2014-02-08 16:18 ` [PATCH 07/11] smp: Consolidate the various smp_call_function_single() declensions Frederic Weisbecker
2014-02-10 20:27   ` Jan Kara
2014-02-08 16:18 ` [PATCH 08/11] smp: Move __smp_call_function_single() below its safe version Frederic Weisbecker
2014-02-10 20:29   ` Jan Kara
2014-02-08 16:18 ` [PATCH 09/11] watchdog: Simplify a little the IPI call Frederic Weisbecker
2014-02-10 12:26   ` Michal Hocko
2014-02-10 14:46   ` Don Zickus
2014-02-08 16:18 ` Frederic Weisbecker [this message]
2014-02-08 16:42   ` [PATCH 10/11] smp: Remove wait argument from __smp_call_function_single() Christoph Hellwig
2014-02-08 16:46     ` Frederic Weisbecker
2014-02-08 16:54       ` Christoph Hellwig
2014-02-08 16:18 ` [PATCH 11/11] smp: Enhance and precise the role & requirements of __smp_call_function_single() Frederic Weisbecker
  -- strict thread matches above, loose matches on Subject: below --
2014-02-24 15:39 [PATCH 00/11] smp: Single IPI cleanups v2 Frederic Weisbecker
2014-02-24 15:40 ` [PATCH 10/11] smp: Remove wait argument from __smp_call_function_single() Frederic Weisbecker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1391876320-25068-11-git-send-email-fweisbec@gmail.com \
    --to=fweisbec@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=hch@infradead.org \
    --cc=jack@suse.cz \
    --cc=jens.axboe@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox