From: "Paul E. McKenney" <paulmck@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: mingo@kernel.org, torvalds@linux-foundation.org,
linux-kernel@vger.kernel.org, will@kernel.org, hch@lst.de,
axboe@kernel.dk, chris@chris-wilson.co.uk, davem@davemloft.net,
kuba@kernel.org, fweisbec@gmail.com, oleg@redhat.com
Subject: Re: [RFC][PATCH 7/9] smp,irq_work: Use the new irq_work API
Date: Wed, 22 Jul 2020 15:09:43 -0700 [thread overview]
Message-ID: <20200722220943.GC23360@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200722153017.445914025@infradead.org>
On Wed, Jul 22, 2020 at 05:01:56PM +0200, Peter Zijlstra wrote:
> Convert the performance sensitive users of
> smp_call_single_function_async() over to the new
> irq_work_queue_remote_static().
>
> The new API is marginally less crap but taking these users away allows
> fixing up smp_call_single_function_async() without risk of performance
> regressions.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
But given that kvm.sh gives a return status like "git bisect run",
why not bisect?
And bisection converged on this patch with a similar splat.
Decoding the assembly makes it appear that nohz_work_func() got a bogus
pointer having a rather odd value of 0x200000001.
Thanx, Paul
------------------------------------------------------------------------
0: e8 35 52 ff ff call 0xffffffffffff523a
5: 48 89 c3 mov rbx,rax
8: eb 93 jmp 0xffffffffffffff9d
a: 8b 43 70 mov eax,DWORD PTR [rbx+0x70]
d: 89 44 24 04 mov DWORD PTR [rsp+0x4],eax
11: eb c2 jmp 0xffffffffffffffd5
13: 48 c7 c0 ea ff ff ff mov rax,0xffffffffffffffea
1a: eb 94 jmp 0xffffffffffffffb0
1c: e8 c9 02 a7 00 call 0xa702ea
21: 66 0f 1f 84 00 00 00 nop WORD PTR [rax+rax*1+0x0]
28: 00 00
2a: 4c 63 8f f0 09 00 00 movsxd r9,DWORD PTR [rdi+0x9f0] <-------
31: 48 c7 c1 00 91 02 00 mov rcx,0x29100
38: 48 89 ca mov rdx,rcx
> ---
> block/blk-mq.c | 8 ++++----
> include/linux/blkdev.h | 4 ++--
> include/linux/netdevice.h | 3 ++-
> kernel/sched/core.c | 16 ++++++++--------
> kernel/sched/fair.c | 6 +++---
> kernel/sched/sched.h | 4 ++--
> net/core/dev.c | 15 ++++++++++-----
> 7 files changed, 31 insertions(+), 25 deletions(-)
>
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -623,9 +623,9 @@ static int blk_softirq_cpu_dead(unsigned
> }
>
>
> -static void __blk_mq_complete_request_remote(void *data)
> +static void __blk_mq_complete_request_remote(struct irq_work *work)
> {
> - struct request *rq = data;
> + struct request *rq = container_of(work, struct request, work);
>
> /*
> * For most of single queue controllers, there is only one irq vector
> @@ -672,8 +672,8 @@ bool blk_mq_complete_request_remote(stru
> return false;
>
> if (blk_mq_complete_need_ipi(rq)) {
> - INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
> - smp_call_function_single_async(rq->mq_ctx->cpu, &rq->csd);
> + rq->work = IRQ_WORK_INIT_HARD(__blk_mq_complete_request_remote);
> + irq_work_queue_remote_static(rq->mq_ctx->cpu, &rq->work);
> } else {
> if (rq->q->nr_hw_queues > 1)
> return false;
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -19,7 +19,7 @@
> #include <linux/stringify.h>
> #include <linux/gfp.h>
> #include <linux/bsg.h>
> -#include <linux/smp.h>
> +#include <linux/irq_work.h>
> #include <linux/rcupdate.h>
> #include <linux/percpu-refcount.h>
> #include <linux/scatterlist.h>
> @@ -234,7 +234,7 @@ struct request {
> unsigned long deadline;
>
> union {
> - struct __call_single_data csd;
> + struct irq_work work;
> u64 fifo_time;
> };
>
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -26,6 +26,7 @@
> #include <linux/delay.h>
> #include <linux/atomic.h>
> #include <linux/prefetch.h>
> +#include <linux/irq_work.h>
> #include <asm/cache.h>
> #include <asm/byteorder.h>
>
> @@ -3126,7 +3127,7 @@ struct softnet_data {
> unsigned int input_queue_head ____cacheline_aligned_in_smp;
>
> /* Elements below can be accessed between CPUs for RPS/RFS */
> - call_single_data_t csd ____cacheline_aligned_in_smp;
> + struct irq_work work ____cacheline_aligned_in_smp;
> struct softnet_data *rps_ipi_next;
> unsigned int cpu;
> unsigned int input_queue_tail;
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -265,9 +265,9 @@ static void __hrtick_restart(struct rq *
> /*
> * called from hardirq (IPI) context
> */
> -static void __hrtick_start(void *arg)
> +static void __hrtick_start(struct irq_work *work)
> {
> - struct rq *rq = arg;
> + struct rq *rq = container_of(work, struct rq, hrtick_work);
> struct rq_flags rf;
>
> rq_lock(rq, &rf);
> @@ -298,7 +298,7 @@ void hrtick_start(struct rq *rq, u64 del
> if (rq == this_rq())
> __hrtick_restart(rq);
> else
> - smp_call_function_single_async(cpu_of(rq), &rq->hrtick_csd);
> + irq_work_queue_remote_static(cpu_of(rq), &rq->hrtick_work);
> }
>
> #else
> @@ -323,7 +323,7 @@ void hrtick_start(struct rq *rq, u64 del
> static void hrtick_rq_init(struct rq *rq)
> {
> #ifdef CONFIG_SMP
> - INIT_CSD(&rq->hrtick_csd, __hrtick_start, rq);
> + rq->hrtick_work = IRQ_WORK_INIT_HARD(__hrtick_start);
> #endif
> hrtimer_init(&rq->hrtick_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
> rq->hrtick_timer.function = hrtick;
> @@ -633,14 +633,14 @@ void wake_up_nohz_cpu(int cpu)
> wake_up_idle_cpu(cpu);
> }
>
> -static void nohz_csd_func(void *info)
> +static void nohz_work_func(struct irq_work *work)
> {
> - struct rq *rq = info;
> + struct rq *rq = container_of(work, struct rq, nohz_work);
> int cpu = cpu_of(rq);
> unsigned int flags;
>
> /*
> - * Release the rq::nohz_csd.
> + * Release rq::nohz_work.
> */
> flags = atomic_fetch_andnot(NOHZ_KICK_MASK, nohz_flags(cpu));
> WARN_ON(!(flags & NOHZ_KICK_MASK));
> @@ -6827,7 +6827,7 @@ void __init sched_init(void)
> rq->last_blocked_load_update_tick = jiffies;
> atomic_set(&rq->nohz_flags, 0);
>
> - INIT_CSD(&rq->nohz_csd, nohz_csd_func, rq);
> + rq->nohz_work = IRQ_WORK_INIT_HARD(nohz_work_func);
> #endif
> #endif /* CONFIG_SMP */
> hrtick_rq_init(rq);
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10039,8 +10039,8 @@ static void kick_ilb(unsigned int flags)
> return;
>
> /*
> - * Access to rq::nohz_csd is serialized by NOHZ_KICK_MASK; he who sets
> - * the first flag owns it; cleared by nohz_csd_func().
> + * Access to rq::nohz_work is serialized by NOHZ_KICK_MASK; he who sets
> + * the first flag owns it; cleared by nohz_work_func().
> */
> flags = atomic_fetch_or(flags, nohz_flags(ilb_cpu));
> if (flags & NOHZ_KICK_MASK)
> @@ -10051,7 +10051,7 @@ static void kick_ilb(unsigned int flags)
> * is idle. And the softirq performing nohz idle load balance
> * will be run before returning from the IPI.
> */
> - smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);
> + irq_work_queue_remote_static(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_work);
> }
>
> /*
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -904,7 +904,7 @@ struct rq {
> #ifdef CONFIG_SMP
> unsigned long last_blocked_load_update_tick;
> unsigned int has_blocked_load;
> - call_single_data_t nohz_csd;
> + struct irq_work nohz_work;
> #endif /* CONFIG_SMP */
> unsigned int nohz_tick_stopped;
> atomic_t nohz_flags;
> @@ -1015,7 +1015,7 @@ struct rq {
>
> #ifdef CONFIG_SCHED_HRTICK
> #ifdef CONFIG_SMP
> - call_single_data_t hrtick_csd;
> + struct irq_work hrtick_work;
> #endif
> struct hrtimer hrtick_timer;
> #endif
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4444,9 +4444,9 @@ EXPORT_SYMBOL(rps_may_expire_flow);
> #endif /* CONFIG_RFS_ACCEL */
>
> /* Called from hardirq (IPI) context */
> -static void rps_trigger_softirq(void *data)
> +static void rps_trigger_softirq(struct irq_work *work)
> {
> - struct softnet_data *sd = data;
> + struct softnet_data *sd = container_of(work, struct softnet_data, work);
>
> ____napi_schedule(sd, &sd->backlog);
> sd->received_rps++;
> @@ -6185,8 +6185,13 @@ static void net_rps_send_ipi(struct soft
> while (remsd) {
> struct softnet_data *next = remsd->rps_ipi_next;
>
> - if (cpu_online(remsd->cpu))
> - smp_call_function_single_async(remsd->cpu, &remsd->csd);
> + if (cpu_online(remsd->cpu)) {
> + /*
> + * XXX can there be two CPUs calling into the same remsd?
> + * XXX serialized by NAPI_STATE_SCHED ??
> + */
> + irq_work_queue_remote_static(remsd->cpu, &remsd->work);
> + }
> remsd = next;
> }
> #endif
> @@ -10661,7 +10666,7 @@ static int __init net_dev_init(void)
> INIT_LIST_HEAD(&sd->poll_list);
> sd->output_queue_tailp = &sd->output_queue;
> #ifdef CONFIG_RPS
> - INIT_CSD(&sd->csd, rps_trigger_softirq, sd);
> + init_irq_work(&sd->work, rps_trigger_softirq);
> sd->cpu = i;
> #endif
>
>
>
next prev parent reply other threads:[~2020-07-22 22:09 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-22 15:01 [RFC][PATCH 0/9] smp: irq_work / smp_call_function rework Peter Zijlstra
2020-07-22 15:01 ` [RFC][PATCH 1/9] irq_work: Cleanup Peter Zijlstra
2020-07-23 16:14 ` Paul E. McKenney
2020-08-17 9:03 ` peterz
2020-08-17 9:16 ` peterz
2020-08-17 13:00 ` Paul E. McKenney
2020-08-18 10:34 ` peterz
2020-07-25 11:58 ` Ingo Molnar
2020-07-25 17:30 ` Peter Zijlstra
2020-07-22 15:01 ` [RFC][PATCH 2/9] smp: Cleanup smp_call_function*() Peter Zijlstra
2020-07-24 18:01 ` Paul E. McKenney
2020-07-22 15:01 ` [RFC][PATCH 3/9] irq_work: Optimize irq_work_single() Peter Zijlstra
2020-07-22 15:01 ` [RFC][PATCH 4/9] irq_work: Unconditionally build on SMP Peter Zijlstra
2020-07-22 15:01 ` [RFC][PATCH 5/9] irq_work: Provide irq_work_queue_remote() Peter Zijlstra
2020-07-22 19:59 ` Paul E. McKenney
2020-07-22 15:01 ` [RFC][PATCH 6/9] irq_work: Provide irq_work_queue_remote_static() Peter Zijlstra
2020-07-22 15:01 ` [RFC][PATCH 7/9] smp,irq_work: Use the new irq_work API Peter Zijlstra
2020-07-22 22:09 ` Paul E. McKenney [this message]
2020-07-22 15:01 ` [RFC][PATCH 8/9] smp: Make smp_call_function_single_async() safer Peter Zijlstra
2020-07-22 15:01 ` [RFC][PATCH 9/9] irq_work: Add a few comments Peter Zijlstra
2020-07-22 20:51 ` [RFC][PATCH 0/9] smp: irq_work / smp_call_function rework Paul E. McKenney
2020-07-22 23:30 ` Peter Zijlstra
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=20200722220943.GC23360@paulmck-ThinkPad-P72 \
--to=paulmck@kernel.org \
--cc=axboe@kernel.dk \
--cc=chris@chris-wilson.co.uk \
--cc=davem@davemloft.net \
--cc=fweisbec@gmail.com \
--cc=hch@lst.de \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
--cc=will@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.