Hi,
Is the mtd system thread safe? It seems it doesn’t have a lock. While mtd_mapper does introduce a lock for the parent it would still be possible to start operations on the mtd and a mtd_mapper device from separate threads.
Thanks,
I’m amazed that the mapper has a lock in the first place. Usually I’d expect that the underlying MTD device would serialize its accesses if that is necessary. (@bergzand might have some insights here).
It may not be necessary for a backend to have an explict mutex – for example, flash pages can be read simultaneously from different threads, one thread’s erase operation would not disturb another thread’s read, and if they happen to hit the same page, then the MCU usually blocks the read until the erase is complete). Looking at sdcard_spi for another backend example, this implicitly locks by performing an spi_acquire on the underlying peripheral.
From that I’d think that the general expectation is for MTD implementations to do locking as necessary. It is not documented, though – so if you want to use MTD a lot from different threads, be invited to open an issue indicating that this should be documented.
As far as I know there is nothing in the MTD docs specifying that thread safety is guaranteed by the underlying MTD device. Two different threads could be working on the separate maps and the lower device doesn’t have this guarantee in place. The mutex here is to ensure accesses of the different slices are serialized.
From a brief chat after today’s coordination call it’s probably best if we just gather a majority opinion on whether or not MTD devices are expected to be thread safe or not, and codify that.
Data points we have so far are that some backends are (by implementation) thread safe, and that some tools on top of MTD pass on that thread unsafety (eg FatFS until recently required all accesses to it through the VFS to be serialized, which again to my knowledge is not something the VFS provides).
One argument I’d like to make before we just move on is that when an MTD is accessed from different threads, the driver has better information about which kind of locking is necessary – as opposed to the user who would (on non-threadsafe MTDs) would need to lock the MTD and thus potentially causes priority inversion even though the device wouldn’t need the lock in the first place.
Because of that, and because the drivers I’ve seen so far are already cheaply thread safe, I prefer the MTD interface to require thread safety from drivers.
To make this more easily visible (without any claims to a poll like this being binding in the project):
- MTD drivers should allow simultaneous access to a device from multiple threads.
- MTD users should ensure that only one thread access a device at a time.
0 voters
Especially pinging @vincent-d and @benpicco for their opinions.
Hi, I am relatively new to riot. As far as I understand mtd is not always directly calling the flash layer, The flash layer should serialize if required. If mtd introduces some kind of caching, mtd is also required to serialize.
If someone were to implement this it would be a mtd caching device that sits on top of a physical device and that implements the MTD device API.
This would need to implement locking, but it would be idependend from the common code in mtd.c that provides the user API.
Doesn’t the read-modify-write method use a kind of cache?
Ah yes you are right, I thought you meant a general read / write cache.
but that method reads the page, modifies it in memory and writes it back to the device - if another thread writes to the device in between we would have a race condition.
Are mtd_t meant to be a shared resource? (Aside from mtd_mapper, which already is guarded by a Mutex.) If MTD is used as a backend for a file system, that file system assumes it’s the one and only accessing the MTD. Does anybody have a counter example?