linux-kernel - Re: [PATCH] uio: Allow to take irq bottom-half into irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Dec 2017 10:16:11 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Andrey Zhizhikin <andrey.z@...il.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] uio: Allow to take irq bottom-half into irq_handler with
 additional dt-binding


A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Wed, Dec 06, 2017 at 05:59:01PM +0100, Andrey Zhizhikin wrote:
> Hello Greg,
> 
> Thanks a lot for your prompt reply!
> 
> First of, this is my first patch submission to the Kernel, so thanks a
> lot for your additional guidelines here regarding missing pieces.
> Please don't judge me hard here. :)

No worries, everyone has to start somewhere :)

> I would add new DT bindings to Documentation and contact DT
> maintainers to have a new binding discussed. However, I was not able
> to find any dt-binding documentation for uio drivers in the kernel,
> presumably I would have to create a new entry there...

The uio-phys driver does use DT bindings, so perhaps look at how those
are defined.

> As for the win against latency and running the patch against the
> system which has all IRQ in threaded mode:
> 
> I've actually originated this patch based on the PREEMPT_RT kernel
> configuration, where all IRQs are threaded. I have ARM-based system
> running around 20 genirq UIO instances, and was demoting 2 of those
> from threaded to non-threaded IRQ handlers without any issues recorded
> to all the IRQ handlers. This patch actually is aimed exactly with the
> logic that if new property is not found - then system behavior is not
> amended, and all IRQs stays threaded. If needed, then a developer can
> enable this property for it's node, but then he should be well-aware
> of what this property implications are.
> 
> In average, using ftrace and kernelshark to analyze I observed the
> gain of 20-30 usec per chain: irq_handler_entry -> irq/XX-uio -> <User
> space IRQ part> so I would say the gain is not very significant for
> average user-space task. However IMHO, there are several hidden
> benefits here with having this modification, namely:
> - It eliminates few re-scheduling operations, making INT ACK code more
> robust and relieves the pressure from the scheduler when HW interrupt
> for this IRQ is signaled at a high-enough frequency;
> - It makes top and bottom half to be executed back-to-back with IRQ
> OFF, making operation pseudo-atomic;
> - Above gain might be significant when average latency times for the
> systems are comparable.

Ok, that all seems like a good thing to have the ability to do here, you
should mention it in the changelog text when you redo this patch.

thanks,

greg k-h