* Re: [patch 3/8] x86/apic: Provide apic_ack_irq()
2018-06-04 15:33 [patch 3/8] x86/apic: Provide apic_ack_irq() Thomas Gleixner
@ 2018-06-05 7:07 ` Song Liu
2018-06-05 11:31 ` Dou Liyang
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Song Liu @ 2018-06-05 7:07 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Ingo Molnar, Peter Zijlstra, Borislav Petkov,
Dmitry Safonov, Tariq Toukan, Joerg Roedel, Mike Travis, stable
On Mon, Jun 4, 2018 at 8:33 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> apic_ack_edge() is explicitely for handling interrupt affinity cleanup when
> interrupt remapping is not available or disable.
>
> Remapped interrupts and also some of the platform specific special
> interrupts, e.g. UV, invoke ack_APIC_irq() directly.
>
> To address the issue of failing an affinity update with -EBUSY the delayed
> affinity mechanism can be reused, but ack_APIC_irq() does not handle
> that. Adding this to ack_APIC_irq() is not possible, because that function
> is also used for exceptions and directly handled interrupts like IPIs.
>
> Create a new function, which just contains the conditional invocation of
> irq_move_irq() and the final ack_APIC_irq(). Making the invocation of
> irq_move_irq() conditional avoids the out of line call if the pending bit
> is not set.
>
> Reuse the new function in apic_ack_edge().
>
> Preparatory change for the real fix
>
> Fixes: dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
Tested-by: Song Liu <songliubraving@fb.com>
> ---
> arch/x86/include/asm/apic.h | 2 ++
> arch/x86/kernel/apic/vector.c | 10 ++++++++--
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(vo
>
> #endif /* CONFIG_X86_LOCAL_APIC */
>
> +extern void apic_ack_irq(struct irq_data *data);
> +
> static inline void ack_APIC_irq(void)
> {
> /*
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -809,11 +809,17 @@ static int apic_retrigger_irq(struct irq
> return 1;
> }
>
> +void apic_ack_irq(struct irq_data *irqd)
> +{
> + if (unlikely(irqd_is_setaffinity_pending(irqd)))
> + irq_move_irq(irqd);
> + ack_APIC_irq();
> +}
> +
> void apic_ack_edge(struct irq_data *irqd)
> {
> irq_complete_move(irqd_cfg(irqd));
> - irq_move_irq(irqd);
> - ack_APIC_irq();
> + apic_ack_irq(irqd);
> }
>
> static struct irq_chip lapic_controller = {
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/8] x86/apic: Provide apic_ack_irq()
2018-06-04 15:33 [patch 3/8] x86/apic: Provide apic_ack_irq() Thomas Gleixner
2018-06-05 7:07 ` Song Liu
@ 2018-06-05 11:31 ` Dou Liyang
2018-06-05 11:41 ` Thomas Gleixner
2018-06-06 13:31 ` [tip:x86/urgent] genirq/migration: Avoid out of line call if pending is not set tip-bot for Thomas Gleixner
2018-06-06 13:32 ` [tip:x86/urgent] x86/apic: Provide apic_ack_irq() tip-bot for Thomas Gleixner
3 siblings, 1 reply; 9+ messages in thread
From: Dou Liyang @ 2018-06-05 11:31 UTC (permalink / raw)
To: Thomas Gleixner, LKML
Cc: Ingo Molnar, Peter Zijlstra, Borislav Petkov, Dmitry Safonov,
Tariq Toukan, Song Liu, Joerg Roedel, Mike Travis, stable
Hi Thomas,
At 06/04/2018 11:33 PM, Thomas Gleixner wrote:
> apic_ack_edge() is explicitely for handling interrupt affinity cleanup when
> interrupt remapping is not available or disable.
>
> Remapped interrupts and also some of the platform specific special
> interrupts, e.g. UV, invoke ack_APIC_irq() directly.
>
> To address the issue of failing an affinity update with -EBUSY the delayed
> affinity mechanism can be reused, but ack_APIC_irq() does not handle
> that. Adding this to ack_APIC_irq() is not possible, because that function
> is also used for exceptions and directly handled interrupts like IPIs.
>
> Create a new function, which just contains the conditional invocation of
> irq_move_irq() and the final ack_APIC_irq(). Making the invocation of
> irq_move_irq() conditional avoids the out of line call if the pending bit
> is not set.
>
> Reuse the new function in apic_ack_edge().
>
> Preparatory change for the real fix
>
> Fixes: dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
> Signed-off-by: Thomas Gleixner<tglx@linutronix.de>
> Cc:stable@vger.kernel.org
> ---
> arch/x86/include/asm/apic.h | 2 ++
> arch/x86/kernel/apic/vector.c | 10 ++++++++--
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(vo
>
> #endif /* CONFIG_X86_LOCAL_APIC */
>
> +extern void apic_ack_irq(struct irq_data *data);
> +
> static inline void ack_APIC_irq(void)
> {
> /*
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -809,11 +809,17 @@ static int apic_retrigger_irq(struct irq
> return 1;
> }
>
> +void apic_ack_irq(struct irq_data *irqd)
> +{
> + if (unlikely(irqd_is_setaffinity_pending(irqd)))
Affinity pending is also judged in
> + irq_move_irq(irqd);
If we can remove the if(...) statement here
Thanks,
dou
> + ack_APIC_irq();
> +}
> +
> void apic_ack_edge(struct irq_data *irqd)
> {
> irq_complete_move(irqd_cfg(irqd));
> - irq_move_irq(irqd);
> - ack_APIC_irq();
> + apic_ack_irq(irqd);
> }
>
> static struct irq_chip lapic_controller = {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/8] x86/apic: Provide apic_ack_irq()
2018-06-05 11:31 ` Dou Liyang
@ 2018-06-05 11:41 ` Thomas Gleixner
2018-06-06 3:48 ` Dou Liyang
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2018-06-05 11:41 UTC (permalink / raw)
To: Dou Liyang
Cc: LKML, Ingo Molnar, Peter Zijlstra, Borislav Petkov,
Dmitry Safonov, Tariq Toukan, Song Liu, Joerg Roedel,
Mike Travis, stable
On Tue, 5 Jun 2018, Dou Liyang wrote:
> > +{
> > + if (unlikely(irqd_is_setaffinity_pending(irqd)))
>
> Affinity pending is also judged in
>
> > + irq_move_irq(irqd);
>
> If we can remove the if(...) statement here
That requires to fix all call sites in ia64 and that's why I didn't. But
we can make irq_move_irq() an inline function and have the check in the
inline.
Thanks,
tglx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/8] x86/apic: Provide apic_ack_irq()
2018-06-05 11:41 ` Thomas Gleixner
@ 2018-06-06 3:48 ` Dou Liyang
2018-06-06 8:04 ` Thomas Gleixner
0 siblings, 1 reply; 9+ messages in thread
From: Dou Liyang @ 2018-06-06 3:48 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Ingo Molnar, Peter Zijlstra, Borislav Petkov,
Dmitry Safonov, Tariq Toukan, Song Liu, Joerg Roedel,
Mike Travis, stable
Hi Thomas,
At 06/05/2018 07:41 PM, Thomas Gleixner wrote:
> On Tue, 5 Jun 2018, Dou Liyang wrote:
>>> +{
>>> + if (unlikely(irqd_is_setaffinity_pending(irqd)))
>>
>> Affinity pending is also judged in
>>
>>> + irq_move_irq(irqd);
>>
>> If we can remove the if(...) statement here
>
> That requires to fix all call sites in ia64 and that's why I didn't. But
I didn't express clearly, I meant remove the if(...) statement from
apic_ack_irq(), it doesn't require to fix the call sites in ia64.
+void apic_ack_irq(struct irq_data *irqd)
+{
+ irq_move_irq(irqd);
+ ack_APIC_irq();
+}
BTW, If apic_ack_irq() can accept _any_ irq_data when hierarchical
irqdomains are enabled[1]? If it is true, If there is a situation in
the original code that we should avoid:
If the top-level irq_data has the IRQD_SETAFFINITY_PENDING state, but
non-top-level irq_data state not, when using non-top-level irq_data in
apic_ack_irq(), we may skip the irq_move_irq() which we should call.
[1] commit 77ed42f18edd("genirq: Prevent crash in irq_move_irq()")
> we can make irq_move_irq() an inline function and have the check in the
> inline.
>
I don't know why do we need to make irq_move_irq() an inline function.
And, yes, irq_move_irq() has already had the check
...
if (likely(!irqd_is_setaffinity_pending(idata)))
return;
...
Thanks,
dou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/8] x86/apic: Provide apic_ack_irq()
2018-06-06 3:48 ` Dou Liyang
@ 2018-06-06 8:04 ` Thomas Gleixner
2018-06-06 8:18 ` Dou Liyang
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2018-06-06 8:04 UTC (permalink / raw)
To: Dou Liyang
Cc: LKML, Ingo Molnar, Peter Zijlstra, Borislav Petkov,
Dmitry Safonov, Tariq Toukan, Song Liu, Joerg Roedel,
Mike Travis, stable
On Wed, 6 Jun 2018, Dou Liyang wrote:
> Hi Thomas,
>
> At 06/05/2018 07:41 PM, Thomas Gleixner wrote:
> > On Tue, 5 Jun 2018, Dou Liyang wrote:
> > > > +{
> > > > + if (unlikely(irqd_is_setaffinity_pending(irqd)))
> > >
> > > Affinity pending is also judged in
> > >
> > > > + irq_move_irq(irqd);
> > >
> > > If we can remove the if(...) statement here
> >
> > That requires to fix all call sites in ia64 and that's why I didn't. But
>
> I didn't express clearly, I meant remove the if(...) statement from
> apic_ack_irq(), it doesn't require to fix the call sites in ia64.
I put the check there on purpose as I explained in the changelog:
Making the invocation of irq_move_irq() conditional avoids the out of
line call if the pending bit is not set.
Thanks,
tglx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/8] x86/apic: Provide apic_ack_irq()
2018-06-06 8:04 ` Thomas Gleixner
@ 2018-06-06 8:18 ` Dou Liyang
0 siblings, 0 replies; 9+ messages in thread
From: Dou Liyang @ 2018-06-06 8:18 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Ingo Molnar, Peter Zijlstra, Borislav Petkov,
Dmitry Safonov, Tariq Toukan, Song Liu, Joerg Roedel,
Mike Travis, stable
Hi Thomas,
At 06/06/2018 04:04 PM, Thomas Gleixner wrote:
> On Wed, 6 Jun 2018, Dou Liyang wrote:
>
>> Hi Thomas,
>>
>> At 06/05/2018 07:41 PM, Thomas Gleixner wrote:
>>> On Tue, 5 Jun 2018, Dou Liyang wrote:
>>>>> +{
>>>>> + if (unlikely(irqd_is_setaffinity_pending(irqd)))
>>>>
>>>> Affinity pending is also judged in
>>>>
>>>>> + irq_move_irq(irqd);
>>>>
>>>> If we can remove the if(...) statement here
>>>
>>> That requires to fix all call sites in ia64 and that's why I didn't. But
>>
>> I didn't express clearly, I meant remove the if(...) statement from
>> apic_ack_irq(), it doesn't require to fix the call sites in ia64.
>
> I put the check there on purpose as I explained in the changelog:
>
> Making the invocation of irq_move_irq() conditional avoids the out of
> line call if the pending bit is not set.
>
I completely understand now, thank you so much. :-)
Thanks,
dou
> Thanks,
>
> tglx
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip:x86/urgent] genirq/migration: Avoid out of line call if pending is not set
2018-06-04 15:33 [patch 3/8] x86/apic: Provide apic_ack_irq() Thomas Gleixner
2018-06-05 7:07 ` Song Liu
2018-06-05 11:31 ` Dou Liyang
@ 2018-06-06 13:31 ` tip-bot for Thomas Gleixner
2018-06-06 13:32 ` [tip:x86/urgent] x86/apic: Provide apic_ack_irq() tip-bot for Thomas Gleixner
3 siblings, 0 replies; 9+ messages in thread
From: tip-bot for Thomas Gleixner @ 2018-06-06 13:31 UTC (permalink / raw)
To: linux-tip-commits
Cc: hpa, linux-kernel, bp, tglx, mike.travis, douly.fnst, peterz,
mingo, liu.song.a23, 0x7f454c46, jroedel, tariqt
Commit-ID: d340ebd696f921d3ad01b8c0c29dd38f2ad2bf3e
Gitweb: https://git.kernel.org/tip/d340ebd696f921d3ad01b8c0c29dd38f2ad2bf3e
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 6 Jun 2018 14:46:59 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 6 Jun 2018 15:18:20 +0200
genirq/migration: Avoid out of line call if pending is not set
The upcoming fix for the -EBUSY return from affinity settings requires to
use the irq_move_irq() functionality even on irq remapped interrupts. To
avoid the out of line call, move the check for the pending bit into an
inline helper.
Preparatory change for the real fix. No functional change.
Fixes: dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <liu.song.a23@gmail.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tariq Toukan <tariqt@mellanox.com>
Cc: Dou Liyang <douly.fnst@cn.fujitsu.com>
Link: https://lkml.kernel.org/r/20180604162224.471925894@linutronix.de
---
include/linux/irq.h | 7 ++++++-
kernel/irq/migration.c | 5 +----
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 65916a305f3d..4e66378f290b 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -551,7 +551,12 @@ extern int irq_affinity_online_cpu(unsigned int cpu);
#endif
#if defined(CONFIG_SMP) && defined(CONFIG_GENERIC_PENDING_IRQ)
-void irq_move_irq(struct irq_data *data);
+void __irq_move_irq(struct irq_data *data);
+static inline void irq_move_irq(struct irq_data *data)
+{
+ if (unlikely(irqd_is_setaffinity_pending(data)))
+ __irq_move_irq(data);
+}
void irq_move_masked_irq(struct irq_data *data);
void irq_force_complete_move(struct irq_desc *desc);
#else
diff --git a/kernel/irq/migration.c b/kernel/irq/migration.c
index 8b8cecd18cce..def48589ea48 100644
--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -91,7 +91,7 @@ void irq_move_masked_irq(struct irq_data *idata)
cpumask_clear(desc->pending_mask);
}
-void irq_move_irq(struct irq_data *idata)
+void __irq_move_irq(struct irq_data *idata)
{
bool masked;
@@ -102,9 +102,6 @@ void irq_move_irq(struct irq_data *idata)
*/
idata = irq_desc_get_irq_data(irq_data_to_desc(idata));
- if (likely(!irqd_is_setaffinity_pending(idata)))
- return;
-
if (unlikely(irqd_irq_disabled(idata)))
return;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [tip:x86/urgent] x86/apic: Provide apic_ack_irq()
2018-06-04 15:33 [patch 3/8] x86/apic: Provide apic_ack_irq() Thomas Gleixner
` (2 preceding siblings ...)
2018-06-06 13:31 ` [tip:x86/urgent] genirq/migration: Avoid out of line call if pending is not set tip-bot for Thomas Gleixner
@ 2018-06-06 13:32 ` tip-bot for Thomas Gleixner
3 siblings, 0 replies; 9+ messages in thread
From: tip-bot for Thomas Gleixner @ 2018-06-06 13:32 UTC (permalink / raw)
To: linux-tip-commits
Cc: jroedel, liu.song.a23, hpa, 0x7f454c46, linux-kernel, tglx, bp,
mike.travis, songliubraving, mingo, tariqt, peterz
Commit-ID: c0255770ccdc77ef2184d2a0a2e0cde09d2b44a4
Gitweb: https://git.kernel.org/tip/c0255770ccdc77ef2184d2a0a2e0cde09d2b44a4
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 4 Jun 2018 17:33:55 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 6 Jun 2018 15:18:20 +0200
x86/apic: Provide apic_ack_irq()
apic_ack_edge() is explicitely for handling interrupt affinity cleanup when
interrupt remapping is not available or disable.
Remapped interrupts and also some of the platform specific special
interrupts, e.g. UV, invoke ack_APIC_irq() directly.
To address the issue of failing an affinity update with -EBUSY the delayed
affinity mechanism can be reused, but ack_APIC_irq() does not handle
that. Adding this to ack_APIC_irq() is not possible, because that function
is also used for exceptions and directly handled interrupts like IPIs.
Create a new function, which just contains the conditional invocation of
irq_move_irq() and the final ack_APIC_irq().
Reuse the new function in apic_ack_edge().
Preparatory change for the real fix.
Fixes: dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Song Liu <songliubraving@fb.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <liu.song.a23@gmail.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tariq Toukan <tariqt@mellanox.com>
Link: https://lkml.kernel.org/r/20180604162224.471925894@linutronix.de
---
arch/x86/include/asm/apic.h | 2 ++
arch/x86/kernel/apic/vector.c | 9 +++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 08acd954f00e..74a9e06b6cfd 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(void (*eoi_write)(u32 reg, u32 v)) {}
#endif /* CONFIG_X86_LOCAL_APIC */
+extern void apic_ack_irq(struct irq_data *data);
+
static inline void ack_APIC_irq(void)
{
/*
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 72b575a0b662..b708f597eee3 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -809,13 +809,18 @@ static int apic_retrigger_irq(struct irq_data *irqd)
return 1;
}
-void apic_ack_edge(struct irq_data *irqd)
+void apic_ack_irq(struct irq_data *irqd)
{
- irq_complete_move(irqd_cfg(irqd));
irq_move_irq(irqd);
ack_APIC_irq();
}
+void apic_ack_edge(struct irq_data *irqd)
+{
+ irq_complete_move(irqd_cfg(irqd));
+ apic_ack_irq(irqd);
+}
+
static struct irq_chip lapic_controller = {
.name = "APIC",
.irq_ack = apic_ack_edge,
^ permalink raw reply related [flat|nested] 9+ messages in thread