linux-kernel - Re: [PATCH 03/15] can: m_can: Cache tx putidx and transmits in flight

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:   Fri, 2 Dec 2022 15:46:30 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Markus Schneider-Pargmann <msp@...libre.com>
Cc:     Chandrasekar Ramakrishnan <rcsekar@...sung.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/15] can: m_can: Cache tx putidx and transmits in flight

On 02.12.2022 09:37:40, Markus Schneider-Pargmann wrote:
> Hi Marc,
> 
> On Thu, Dec 01, 2022 at 12:14:50PM +0100, Marc Kleine-Budde wrote:
> > On 16.11.2022 21:52:56, Markus Schneider-Pargmann wrote:
> > > On peripheral chips every read/write can be costly. Avoid reading easily
> > > trackable information and cache them internally. This saves multiple
> > > reads.
> > > 
> > > Transmit FIFO put index is cached, this is increased for every time we
> > > enqueue a transmit request.
> > > 
> > > The transmits in flight is cached as well. With each transmit request it
> > > is increased when reading the finished transmit event it is decreased.
> > > 
> > > A submit limit is cached to avoid submitting too many transmits at once,
> > > either because the TX FIFO or the TXE FIFO is limited. This is currently
> > > done very conservatively as the minimum of the fifo sizes. This means we
> > > can reach FIFO full events but won't drop anything.
> > 
> > You have a dedicated in_flight variable, which is read-modify-write in 2
> > different code path, i.e. this looks racy.
> 
> True, of course, thank you. Yes I have to redesign this a bit for
> concurrency.
> 
> > If you allow only power-of-two FIFO size, you can use 2 unsigned
> > variables, i.e. a head and a tail pointer. You can apply a mask to get
> > the index to the FIFO. The difference between head and tail is the fill
> > level of the FIFO. See mcp251xfd driver for this.
> 
> Maybe that is a trivial question but what's wrong with using atomics
> instead?

I think it's Ok to use an atomic for the fill level. The put index
doesn't need to be. No need to cache the get index, as it's in the same
register as the fill level.

As the mcp251xfd benefits from caching both indexes, a head and tail
pointer felt like the right choice. As both are only written in 1
location, no need for atomics or locking.

> The tcan mram size is limited to 2048 so I would like to avoid limiting
> the possible sizes of the tx fifos.

What FIFO sizes are you using currently?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)