Re: spi-nor: excessive locking in spi_nor_erase()

From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
To: "pratyush@kernel.org" <pratyush@kernel.org>
Cc: "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>
Subject: Re: spi-nor: excessive locking in spi_nor_erase()
Date: Fri, 19 Apr 2024 13:14:54 +0000	[thread overview]
Message-ID: <f7ec2deb3c16ddd7f49d28ddbadd0c7224176593.camel@infinera.com> (raw)
In-Reply-To: <mafs0y1995x2c.fsf@kernel.org>

On Fri, 2024-04-19 at 14:58 +0200, Pratyush Yadav wrote:
> On Wed, Apr 17 2024, Joakim Tjernlund wrote:
> 
> [...]
> > > (spanning multiple sectors) is running, other tasks do still get to run
> > > (since spi_nor_wait_till_ready() calls cond_resched()) but any task that
> > > tries to access the rootfs would have to freeze since the lock is held.
> > > 
> > > Dropping and re-acquiring the lock after erasing each sector or
> > > programming each page would let readers make progress in between. This
> > > shouldn't be too difficult to implement I reckon. You already mostly do
> > > it in your patch below.
> > 
> > Yes, that is simple.
> > 
> > > 
> > > Doing erase or program suspend would only make sense for flashes with
> > > larger sectors since even one sector erase would take a long time. For
> > > those, I suppose we could give reads priority over program or erase
> > > operations. When a read comes in, it suspends the erase, does the read,
> > > and then resumes it. This would be a little bit more complex to
> > > implement.
> > 
> > Right, a bit harder but I think it is needed like cfi_cmdset_0001 do: suspend erase to do read/write
> > Don't think cfi_cmdset suspends writes, that seems overkill.
> > > 
> > > I would suggest you try the former first and see if it already gives you
> > > the results you need. From your patch below, it seems it does. So
> > > perhaps just cleaning it up and turning it onto a proper patch would do
> > > the job for you.
> > 
> > Tests with my patch shows vastly improved responsivity but there is still some
> > way to go. Consider that an erase takes some 0.20-0.25 secs for us in best case in
> > which apps are blocked. We do get some complaints from the app taking too long to complete
> > some tasks during erase. 
> 
> Send some patches to fix this then :-)
> 

Could send these for 5.15.x/6.1.x but current release is different, still want those ?
I am heading out for the weekend so next week in that case.

    Jocke
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/