/cc @crosbymichael @mlaventure @tonistiigi

RWMutex will be really helpful, thanks!

I'm not sure what the read lock is really helping here.

The way I see it, read lock is useful for:

1. Things that are slow to read
2. Recursive read locks

All these operations are really fast, and unless I'm mistaken we aren't really doing any recursive locking here.
Also been down this rabbit hole of making the state lock a read lock.

Slow reads are not necessarily a good indicator to use read locks. A good indicator is the level of read contention. If there are fast reads but they happen a lot frequently then it results in high contention. If there is contention and kernel(and go scheduler) had to be involved then it does result in poor performance. So if there is high read contention and number of reads far outweigh the number of writes, I'd say RWMutex is a pretty good optimization.

I don't think that's the case here. Very unlikely to have more than a few things reading at the same time.

Even if at least two go routines contend for this lock very frequently for just reading then this PR will help. But that can be known only by profiling.

I think there can be multiple concurrent calls to SystemInfo() mainly due to some poorly-implemented 3rd party tool, but if we can't reach consensus on using RWMutex, I can revert to the normal Mutex 😃 .

@mrjana @cpuguy83 @LK4D4 what's the conclusion here?

Personally I don't mind this being an RWMutex even if there are less frequent reads. But looks like @cpuguy83 had some experience doing the same some time before. So it depends on what his experience was.

I think it's unnecessary and will probably do more harm than good.
This lock should really go away soon-ish.

OK, reverted RWMutex to normal Mutex

Thanks. Thinking about this.. the PR will make calls to docker info get stuck in cases where the container mutex is stuck.
It may be worthwhile to fix this race by keeping a running count of each reported state.... and faster to when there are a large number of containers.

Updated, please revert the label to status/1-design-review and review the design?

ping @LK4D4 @tonistiigi ptal!

I don't think monitor is the right place for storing the state from the cache. It should be covered by the methods that modify state. The same optimization should cover docker ps use cases as well, not only docker info. Currently in docker info we don't take a lock causing a data race, and in docker ps take full container lock, causing ps to freeze when this lock is taken for a long time(start, stop). In both cases we should just return the last known state and make switches between states atomic.

@tonistiigi Thank you for looking into this.
However, I think @cpuguy83 is going to significantly refactor the container structure (#26668 (comment), also heard concrete idea from him at Berlin last week 🍻 )

So I'm closing this PR at the moment.
However, if migrating to the new container structure takes a while, I'd like to reopen this PR and update according to @tonistiigi 's comment.

PTAL @cpuguy83