On Thu, Jul 09, 2020 at 06:36:21PM +0800, Gao Xiang wrote: > Hi Dave, > > On Thu, Jul 09, 2020 at 12:32:46PM +1000, Dave Chinner wrote: > > ... > > > > > if (trylock AGI) > > > > return; > > > > > > (adding some notes here, this patch doesn't use try lock here > > > finally but unlock perag and take AGI and relock and recheck tail_empty.... > > > since the tail non-empty is rare...) > > > > *nod* > > > > My point was largely that this sort of thing is really obvious and > > easy to optimise once the locking is cleanly separated. Adding a > > trylock rather than drop/relock is another patch for the series :P > > I thought trylock way yesterday as well... > Apart from that we don't have the AGI trylock method yet, it seems > not work as well... > > Thinking about one thread is holding a AGI lock and wants to lock perag. > And another thread holds a perag, but the trylock will fail and the second > wait-lock will cause deadlock... like below: > > Thread 1 Thread 2 > lock AGI > lock perag > lock perag > trylock AGI > lock AGI <- dead lock here > > And trylock perag instead seems not work well as well, since it fails > more frequently so we need lock AGI and then lock perag as a fallback. > > So currently, I think it's better to use unlock, do something, regrab, > recheck method for now... > > And yeah, that can be done in another patch if some better way. (e.g. > moving modifing AGI into pre-commit, so we can only take one per-ag > lock here...) Oh, sorry, I didn't notice the words > /* > * Slow path, need to lock AGI first. Don't even bother > * rechecking tail pointers or trying to optimise for > * minimal AGI lock hold time as racing unlink list mods > * will all block on the perag lock once we take that. They > * will then hit the !tail empty fast path and not require > * the AGI lock at all. > */ If we'd only try to trylock AGI and release perag and relock again, and that's fine. Sorry about the noise :) btw, also add some other notes here. For AGI pre-commits, I'm not quite sure if some users take another locked buffer first (but without AGI), so it would have some potential locking order concern as well... But let us think about them later :) add agi to precommit lock other buffers precommit --- lock AGI Thanks, Gao Xiang > > Thanks, > Gao Xiang > > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@xxxxxxxxxxxxx > >
