linux-kernel - Re: [PATCH v1] counter: interrupt-cnt: add counter_push

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] [day] [month] [year] [list]

Message-ID: <20220203104036.GB12695@pengutronix.de>
Date:   Thu, 3 Feb 2022 11:40:36 +0100
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     William Breathitt Gray <vilhelm.gray@...il.com>
Cc:     David Lechner <david@...hnology.com>, linux-iio@...r.kernel.org,
        Robin van der Gracht <robin@...tonic.nl>,
        David Jander <david@...tonic.nl>, linux-kernel@...r.kernel.org,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

Hi William,

On Thu, Feb 03, 2022 at 04:50:54PM +0900, William Breathitt Gray wrote:
> > Hm...
> > 
> > To detect pulse frequency, I need a burst of sequential time-stamps
> > without drops. In case the pulse frequency is higher then the use space
> > is able to get it out of FIFO, we will get high number of drops. 
> > So, we do not need all time stamps. Only bunch of them without drops in
> > the middle.
> > 
> > I know, at some frequency we wont be able to collect all pulses any way.
> > Internal FIFO is just increasing the max detectable frequency. So, it is
> > sort of optimization.
> > 
> > My current driver version has own FIFO which is filled directly by the
> > IRQ handler and user space trigger flush_cb to push all collected
> > time stamps. The main question is: how the flush procedure should be
> > controlled. We have following options:
> > 
> > - Attach it to the read(). The disadvantage: at high frequencies, we
> >   wont be able to get a burst with time stamps without drops in the
> >   middle
> > - Trigger flush from user space. In this case, we make user space a bit
> >   more complicated and cant really get all advantages of poll().
> > - kernel driver is using own timer to trigger flush. The timer can be
> >   configured from user space. The advantage of it, the user space is
> >   simple and has full advantage of using poll()
> > 
> > Regards,
> > Oleksij
> 
> Hi Oleksij,
> 
> Earlier in this thread, Jonathan Cameron suggested using the RCU macros
> to protect access to the events. Taking an RCU approach would eliminate
> the need for spinlocks because the memory barriers are built-in to the
> macros, so I assume flushing would no longer be necessary. Would RCU be
> a viable solution for your needs?

IMO, RCU is the wrong word in this context. It provide an advantage
where we need to reuse/read less frequently changed data. In this use
case we need to move data ASAP, so KFIFO seems to work just fine here.

In any case, after implementi double FIFO and more testing I would
prefer to stay with my initial patch. On a single core system, with have
no waiting time at all. No concurrent access. And on the SMP system
(iMX6Q), currently I can measure higher frequency with initial not
optimized driver:
- with counter_push_event() directly from IRQ: max freq 30-35kHz
- with double FIFO, i have max freq of ~25kHz

Your suggestion was to add COUNTER_EVENT_CHANGE_OF_STATE and use it for
my use case. Correct?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |