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

Groups > linux.kernel > #1260906 > unrolled thread

Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa()  api Thomas Gleixner <tglx@linutronix.de> - 2015-11-02 20:40 +0100
    Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new  generic_handle_irq_rt_wa() api Grygorii Strashko <grygorii.strashko@ti.com> - 2015-11-03 20:20 +0100
      Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa()  api Thomas Gleixner <tglx@linutronix.de> - 2015-11-03 21:00 +0100
        Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new  generic_handle_irq_rt_wa() api Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-11-03 21:20 +0100
          Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new  generic_handle_irq_rt_wa() api Grygorii Strashko <grygorii.strashko@ti.com> - 2015-11-05 17:50 +0100

#1260906 — Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

On Mon, 2 Nov 2015, Grygorii Strashko wrote:
> Now in kernel below code pattern is used by many drivers:
> static irqreturn_t driver_xx_hw_irq_handler(int irq, void *arg)
> {
> 	<read IRQ status register>
>   	<perform HW specific operations>
> 
> 	for (<each set bit in IRQ status register>) {
> 		<get Linux IRQ number>
> 		generic_handle_irq(<Linux IRQ number>);
> 		|- handle_simple_irq()
> 		|-or- handle_level_irq()
> 		|-or- handle_edge_irq()
> 		   |-handle_irq_event()
> 		     |-handle_irq_event_percpu()
> ===
> "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
>  irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts"
> ===
> 	}
> }
> 
> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
> will be forced threaded (by default) and, as result, generic_handle_irq()
> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).
> 
> Instead of spreading this W/A directly in many drivers this patch
> introduces -RT specific version of generic_handle_irq() API -
> generic_handle_irq_rt_wa(). This new generic_handle_irq_rt_wa() just calls
> generic_handle_irq() surrounded by raw_spin_lock_irqsave/irqrestore().
> If -RT is disabled It will fallback to generic_handle_irq().

Why aren't you simply marking these demultiplex handlers with IRQ_NO_THREAD?

Thanks,

	tglx
--
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]

#1261804 — Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

On 11/02/2015 09:38 PM, Thomas Gleixner wrote:
> On Mon, 2 Nov 2015, Grygorii Strashko wrote:
>> Now in kernel below code pattern is used by many drivers:
>> static irqreturn_t driver_xx_hw_irq_handler(int irq, void *arg)
>> {
>> 	<read IRQ status register>
>>    	<perform HW specific operations>
>>
>> 	for (<each set bit in IRQ status register>) {
>> 		<get Linux IRQ number>
>> 		generic_handle_irq(<Linux IRQ number>);
>> 		|- handle_simple_irq()
>> 		|-or- handle_level_irq()
>> 		|-or- handle_edge_irq()
>> 		   |-handle_irq_event()
>> 		     |-handle_irq_event_percpu()
>> ===
>> "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
>>   irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts"
>> ===
>> 	}
>> }
>>
>> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
>> will be forced threaded (by default) and, as result, generic_handle_irq()
>> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
>> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).
>>
>> Instead of spreading this W/A directly in many drivers this patch
>> introduces -RT specific version of generic_handle_irq() API -
>> generic_handle_irq_rt_wa(). This new generic_handle_irq_rt_wa() just calls
>> generic_handle_irq() surrounded by raw_spin_lock_irqsave/irqrestore().
>> If -RT is disabled It will fallback to generic_handle_irq().
> 
> Why aren't you simply marking these demultiplex handlers with IRQ_NO_THREAD?
> 

In general, it's possible. But, in this case, worst scenario will look like:
dra7xx_pcie_msi_irq_handler()
-> dw_handle_msi_irq()
   [code simplified]
   -> for (i = 0; i < MAX_MSI_IRQS; i++) {
	...
	generic_handle_irq(Y(i));
	...
   }
where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.

Thanks.
-- 
regards,
-grygorii
--
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]

#1261835

On Tue, 3 Nov 2015, Grygorii Strashko wrote:
> On 11/02/2015 09:38 PM, Thomas Gleixner wrote:
> > 
> > Why aren't you simply marking these demultiplex handlers with IRQ_NO_THREAD?
> > 
> In general, it's possible. But, in this case, worst scenario will look like:

> dra7xx_pcie_msi_irq_handler()
> -> dw_handle_msi_irq()
>    [code simplified]
>    -> for (i = 0; i < MAX_MSI_IRQS; i++) {
> 	...
> 	generic_handle_irq(Y(i));
> 	...
>    }
> where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.

And you really oversimplified the code above. The reality is:

    for (i = 0; i < MAX_MSI_CTRLS: i++) {
    	u32 status = read_msi_ctrl(i);

	for_each_bit(status)
		handle_irq();
    }

So sure, the worst case here is MAX_MSI_CTRLS * 32, but if all
possible 256 MSI interrupts are pending at the same time, you have
other problems than that.

In the current configuration (32 interrupts), which cannot change
because it's hardwired in silicon, this is a single status read and
assuming that only a few (most of the time it will be exactly ONE) of
those interrupts are pending at the same time is pretty much a sane
assumption. If it wouldn't be then all users of chained interrupt
handlers which usually demultiplex 32 interrupts would suffer from
that problem already.

Aside of that, you would prevent that any of these PCIe interrupts can
be utilized as a "fast" non threaded interrupt on RT. And that I would
consider a real bad limitation for no value. 

MSI has been invented to overcome the issues of wired interrupts
(demultiplexing and sharing), so I don't know why the involved
hardware designers came to the conclusion that demultiplexing MSI
interrupts in software is a sane approach. But then I really gave up
trying to understand hardware designers long ago.

The only sane way to deal with that is to actually mark those handlers
NOTRHEAD and document the limitations of your hardware, so your
customers won't trip over it. If they insist on having 32 MSI
producers on that PCIe bus and make them fire all at the same time,
then you still can provide them your "solution".

Just face it, it's a bad hardware design decision and adding a half
baken hackery which actually hurts sane use cases is not making it any
better.

Thanks,

	tglx





--
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]

#1261844 — Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

On 11/03/2015 08:51 PM, Thomas Gleixner wrote:
>> where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.
> 
> And you really oversimplified the code above. The reality is:
> 
>     for (i = 0; i < MAX_MSI_CTRLS: i++) {
>     	u32 status = read_msi_ctrl(i);
> 
> 	for_each_bit(status)
> 		handle_irq();
>     }
> 
> So sure, the worst case here is MAX_MSI_CTRLS * 32, but if all
> possible 256 MSI interrupts are pending at the same time, you have
> other problems than that.

With threaded interrupts we would have 256 invocations of
wake_up_process() so nothing should take ages.

> Thanks,
> 
> 	tglx

Sebastian
--
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]

#1263414 — Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

Hi Thomas, Sebastian,

On 11/03/2015 10:18 PM, Sebastian Andrzej Siewior wrote:
> On 11/03/2015 08:51 PM, Thomas Gleixner wrote:
>>> where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.
>>
>> And you really oversimplified the code above. The reality is:
>>
>>      for (i = 0; i < MAX_MSI_CTRLS: i++) {
>>      	u32 status = read_msi_ctrl(i);
>>
>> 	for_each_bit(status)
>> 		handle_irq();
>>      }
>>
>> So sure, the worst case here is MAX_MSI_CTRLS * 32, but if all
>> possible 256 MSI interrupts are pending at the same time, you have
>> other problems than that.
>
> With threaded interrupts we would have 256 invocations of
> wake_up_process() so nothing should take ages.
>

Thanks a lot for your time and comments - I'll follow your
recommendations and use IRQF_NO_THREAD.

-- 
regards,
-grygorii
--
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web