The current implementation utilizes a bitmap for managing IRQ resend
handlers, which is allocated based on the SPARSE_IRQ/NR_IRQS macros.
However, this method may not efficiently utilize memory during runtime,
particularly when IRQ_BITMAP_BITS is large.
This proposed patch aims to address this issue by using hlist to manage
IRQ resend handlers instead of relying on static memory allocation.
Additionally, a new function, clear_irq_resend(), is introduced and
called from irq_shutdown to ensure a graceful teardown of IRQD.
Signed-off-by: Shanker Donthineni <[email protected]>
---
include/linux/irqdesc.h | 3 +++
kernel/irq/chip.c | 1 +
kernel/irq/internals.h | 1 +
kernel/irq/irqdesc.c | 6 ++++++
kernel/irq/resend.c | 36 +++++++++++++++++++++---------------
5 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 844a8e30e6de..d9451d456a73 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -102,6 +102,9 @@ struct irq_desc {
int parent_irq;
struct module *owner;
const char *name;
+#ifdef CONFIG_HARDIRQS_SW_RESEND
+ struct hlist_node resend_node;
+#endif
} ____cacheline_internodealigned_in_smp;
#ifdef CONFIG_SPARSE_IRQ
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 49e7bc871fec..2eac5532c3c8 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -306,6 +306,7 @@ static void __irq_disable(struct irq_desc *desc, bool mask);
void irq_shutdown(struct irq_desc *desc)
{
if (irqd_is_started(&desc->irq_data)) {
+ clear_irq_resend(desc);
desc->depth = 1;
if (desc->irq_data.chip->irq_shutdown) {
desc->irq_data.chip->irq_shutdown(&desc->irq_data);
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 5fdc0b557579..2fd17057ed4b 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -113,6 +113,7 @@ irqreturn_t handle_irq_event(struct irq_desc *desc);
/* Resending of interrupts :*/
int check_irq_resend(struct irq_desc *desc, bool inject);
+void clear_irq_resend(struct irq_desc *desc);
bool irq_wait_for_poll(struct irq_desc *desc);
void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action);
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index fd0996274401..21a968bc468b 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -415,6 +415,9 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
desc_set_defaults(irq, desc, node, affinity, owner);
irqd_set(&desc->irq_data, flags);
kobject_init(&desc->kobj, &irq_kobj_type);
+#ifdef CONFIG_HARDIRQS_SW_RESEND
+ INIT_HLIST_NODE(&desc->resend_node);
+#endif
return desc;
@@ -581,6 +584,9 @@ int __init early_irq_init(void)
mutex_init(&desc[i].request_mutex);
init_waitqueue_head(&desc[i].wait_for_threads);
desc_set_defaults(i, &desc[i], node, NULL, NULL);
+#ifdef CONFIG_HARDIRQS_SW_RESEND
+ INIT_HLIST_NODE(&desc->resend_node);
+#endif
}
return arch_early_irq_init();
}
diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index 0c46e9fe3a89..f2b23871cbef 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -21,8 +21,9 @@
#ifdef CONFIG_HARDIRQS_SW_RESEND
-/* Bitmap to handle software resend of interrupts: */
-static DECLARE_BITMAP(irqs_resend, IRQ_BITMAP_BITS);
+/* hlist_head to handle software resend of interrupts: */
+static HLIST_HEAD(irq_resend_list);
+static DEFINE_RAW_SPINLOCK(irq_resend_lock);
/*
* Run software resends of IRQ's
@@ -30,18 +31,16 @@ static DECLARE_BITMAP(irqs_resend, IRQ_BITMAP_BITS);
static void resend_irqs(struct tasklet_struct *unused)
{
struct irq_desc *desc;
- int irq;
-
- while (!bitmap_empty(irqs_resend, nr_irqs)) {
- irq = find_first_bit(irqs_resend, nr_irqs);
- clear_bit(irq, irqs_resend);
- desc = irq_to_desc(irq);
- if (!desc)
- continue;
+ struct hlist_node *n;
+
+ raw_spin_lock_irq(&irq_resend_lock);
+ hlist_for_each_entry_safe(desc, n, &irq_resend_list, resend_node) {
+ hlist_del_init(&desc->resend_node);
local_irq_disable();
desc->handle_irq(desc);
local_irq_enable();
}
+ raw_spin_unlock_irq(&irq_resend_lock);
}
/* Tasklet to handle resend: */
@@ -49,8 +48,6 @@ static DECLARE_TASKLET(resend_tasklet, resend_irqs);
static int irq_sw_resend(struct irq_desc *desc)
{
- unsigned int irq = irq_desc_get_irq(desc);
-
/*
* Validate whether this interrupt can be safely injected from
* non interrupt context
@@ -70,16 +67,25 @@ static int irq_sw_resend(struct irq_desc *desc)
*/
if (!desc->parent_irq)
return -EINVAL;
- irq = desc->parent_irq;
}
- /* Set it pending and activate the softirq: */
- set_bit(irq, irqs_resend);
+ /* Add to resend_list and activate the softirq: */
+ raw_spin_lock(&irq_resend_lock);
+ hlist_add_head(&desc->resend_node, &irq_resend_list);
+ raw_spin_unlock(&irq_resend_lock);
tasklet_schedule(&resend_tasklet);
return 0;
}
+void clear_irq_resend(struct irq_desc *desc)
+{
+ raw_spin_lock(&irq_resend_lock);
+ hlist_del_init(&desc->resend_node);
+ raw_spin_unlock(&irq_resend_lock);
+}
#else
+void clear_irq_resend(struct irq_desc *desc) {}
+
static int irq_sw_resend(struct irq_desc *desc)
{
return -EINVAL;
--
2.25.1
On Sun, Jan 29 2023 at 18:57, Shanker Donthineni wrote:
> +/* hlist_head to handle software resend of interrupts: */
> +static HLIST_HEAD(irq_resend_list);
> +static DEFINE_RAW_SPINLOCK(irq_resend_lock);
>
> /*
> * Run software resends of IRQ's
> @@ -30,18 +31,16 @@ static DECLARE_BITMAP(irqs_resend, IRQ_BITMAP_BITS);
> static void resend_irqs(struct tasklet_struct *unused)
> {
> struct irq_desc *desc;
> - int irq;
> -
> - while (!bitmap_empty(irqs_resend, nr_irqs)) {
> - irq = find_first_bit(irqs_resend, nr_irqs);
> - clear_bit(irq, irqs_resend);
> - desc = irq_to_desc(irq);
> - if (!desc)
> - continue;
> + struct hlist_node *n;
> +
> + raw_spin_lock_irq(&irq_resend_lock);
> + hlist_for_each_entry_safe(desc, n, &irq_resend_list, resend_node) {
> + hlist_del_init(&desc->resend_node);
> local_irq_disable();
> desc->handle_irq(desc);
> local_irq_enable();
> }
> + raw_spin_unlock_irq(&irq_resend_lock);
The lock ordering is broken here. irq_sw_resend() is invoked with
desc->lock held and takes irq_resend_lock.
Lockdep clearly would have told you...
raw_spin_lock_irq(&irq_resend_lock);
while (!hlist_empty(...)) {
desc = hlist_entry(irq_resend_list.first, ...);
hlist_del_init(&desc->resend_node);
raw_spin_unlock(&...);
desc->handle_irq();
raw_spin_lock(&...);
}
Hmm?
Thanks,
tglx
On 1/31/23 02:59, Thomas Gleixner wrote:
> On Sun, Jan 29 2023 at 18:57, Shanker Donthineni wrote:
>> +/* hlist_head to handle software resend of interrupts: */
>> +static HLIST_HEAD(irq_resend_list);
>> +static DEFINE_RAW_SPINLOCK(irq_resend_lock);
>>
>> /*
>> * Run software resends of IRQ's
>> @@ -30,18 +31,16 @@ static DECLARE_BITMAP(irqs_resend, IRQ_BITMAP_BITS);
>> static void resend_irqs(struct tasklet_struct *unused)
>> {
>> struct irq_desc *desc;
>> - int irq;
>> -
>> - while (!bitmap_empty(irqs_resend, nr_irqs)) {
>> - irq = find_first_bit(irqs_resend, nr_irqs);
>> - clear_bit(irq, irqs_resend);
>> - desc = irq_to_desc(irq);
>> - if (!desc)
>> - continue;
>> + struct hlist_node *n;
>> +
>> + raw_spin_lock_irq(&irq_resend_lock);
>> + hlist_for_each_entry_safe(desc, n, &irq_resend_list, resend_node) {
>> + hlist_del_init(&desc->resend_node);
>> local_irq_disable();
>> desc->handle_irq(desc);
>> local_irq_enable();
>> }
>> + raw_spin_unlock_irq(&irq_resend_lock);
> The lock ordering is broken here. irq_sw_resend() is invoked with
> desc->lock held and takes irq_resend_lock.
>
> Lockdep clearly would have told you...
>
> raw_spin_lock_irq(&irq_resend_lock);
> while (!hlist_empty(...)) {
> desc = hlist_entry(irq_resend_list.first, ...);
> hlist_del_init(&desc->resend_node);
> raw_spin_unlock(&...);
> desc->handle_irq();
> raw_spin_lock(&...);
> }
Thanks for catching mistakes, I'll change it to this, please correct me if
I'm doing another mistake.
static void resend_irqs(struct tasklet_struct *unused)
{
struct irq_desc *desc;
raw_spin_lock_irq(&irq_resend_lock);
while (hlist_empty(&irq_resend_list)) {
desc = hlist_entry(irq_resend_list.first, struct irq_desc,
resend_node);
hlist_del_init(&desc->resend_node);
raw_spin_unlock(&irq_resend_lock);
desc->handle_irq(desc);
raw_spin_lock(&irq_resend_lock);
}
raw_spin_unlock_irq(&irq_resend_lock);
}
Thanks,
Shanker
On 1/31/23 10:17, Shanker Donthineni wrote:
>
>
> On 1/31/23 02:59, Thomas Gleixner wrote:
>> On Sun, Jan 29 2023 at 18:57, Shanker Donthineni wrote:
>>> +/* hlist_head to handle software resend of interrupts: */
>>> +static HLIST_HEAD(irq_resend_list);
>>> +static DEFINE_RAW_SPINLOCK(irq_resend_lock);
>>>
>>> /*
>>> * Run software resends of IRQ's
>>> @@ -30,18 +31,16 @@ static DECLARE_BITMAP(irqs_resend, IRQ_BITMAP_BITS);
>>> static void resend_irqs(struct tasklet_struct *unused)
>>> {
>>> struct irq_desc *desc;
>>> - int irq;
>>> -
>>> - while (!bitmap_empty(irqs_resend, nr_irqs)) {
>>> - irq = find_first_bit(irqs_resend, nr_irqs);
>>> - clear_bit(irq, irqs_resend);
>>> - desc = irq_to_desc(irq);
>>> - if (!desc)
>>> - continue;
>>> + struct hlist_node *n;
>>> +
>>> + raw_spin_lock_irq(&irq_resend_lock);
>>> + hlist_for_each_entry_safe(desc, n, &irq_resend_list, resend_node) {
>>> + hlist_del_init(&desc->resend_node);
>>> local_irq_disable();
>>> desc->handle_irq(desc);
>>> local_irq_enable();
>>> }
>>> + raw_spin_unlock_irq(&irq_resend_lock);
>> The lock ordering is broken here. irq_sw_resend() is invoked with
>> desc->lock held and takes irq_resend_lock.
>>
>> Lockdep clearly would have told you...
>>
>> raw_spin_lock_irq(&irq_resend_lock);
>> while (!hlist_empty(...)) {
>> desc = hlist_entry(irq_resend_list.first, ...);
>> hlist_del_init(&desc->resend_node);
>> raw_spin_unlock(&...);
>> desc->handle_irq();
>> raw_spin_lock(&...);
>> }
>
> Thanks for catching mistakes, I'll change it to this, please correct me if
> I'm doing another mistake.
>
> static void resend_irqs(struct tasklet_struct *unused)
> {
> struct irq_desc *desc;
>
> raw_spin_lock_irq(&irq_resend_lock);
> while (hlist_empty(&irq_resend_list)) {
Sorry typo "while (!hlist_empty(&irq_resend_list)) {"
> desc = hlist_entry(irq_resend_list.first, struct irq_desc,
> resend_node);
> hlist_del_init(&desc->resend_node);
> raw_spin_unlock(&irq_resend_lock);
> desc->handle_irq(desc);
> raw_spin_lock(&irq_resend_lock);
> }
> raw_spin_unlock_irq(&irq_resend_lock);
> }
>
> Thanks,
> Shanker