Hi Maarten!
Broadening the audience a bit..
On 9/14/12 9:12 AM, Maarten Lankhorst wrote:
Op 13-09-12 23:00, Thomas Hellstrom schreef:
On 09/13/2012 07:13 PM, Maarten Lankhorst wrote:
Hey
Op 13-09-12 18:41, Thomas Hellstrom schreef:
On 09/13/2012 05:19 PM, Maarten Lankhorst wrote:
Hey,
Op 12-09-12 15:28, Thomas Hellstrom schreef:
On 09/12/2012 02:48 PM, Maarten Lankhorst wrote:
Hey Thomas,
I'm playing around with moving reservations from ttm to global, but how ttm
ttm is handling reservations is getting in the way. The code wants to move
the bo from the lru lock at the same time a reservation is made, but that
seems to be slightly too strict. It would really help me if that guarantee
is removed.
Hi, Maarten.
Removing that restriction is not really possible at the moment.
Also the memory accounting code depends on this, and may cause reservations
in the most awkward places. Since these reservations don't have a ticket
they may and will cause deadlocks. So in short the restriction is there
to avoid deadlocks caused by ticketless reservations.
I have finished the lockdep annotations now which seems to catch almost
all abuse I threw at it, so I'm feeling slightly more confident about moving
the locking order and reservations around.
Maarten, moving reservations in TTM out of the lru lock is incorrect as the
code is
written now. If we want to move it out we need something for ticketless
reservations
I've been thinking of having a global hash table of tickets with the task
struct pointer as the key,
but even then, we'd need to be able to handle EBUSY errors on every operation
that might try to
reserve a buffer.
The fact that lockdep doesn't complain isn't enough. There *will* be deadlock
use-cases when TTM is handed
the right data-set.
Isn't there a way that a subsystem can register a callback to be performed to
remove stuff from LRU and
to take a pre-reservation lock?
What if multiple subsystems need those? You will end up with a deadlock again.
I think it would be easier to change the code in ttm_bo.c to not assume the
first
item on the lru list is really the least recently used, and assume the first
item
that can be reserved without blocking IS the least recently used instead.
So what would happen then is that we'd spin on the first item on the LRU list,
since
when reserving we must release the LRU lock, and if reserving fails, we thus
need to restart LRU traversal. Typically after a schedule(). That's bad.
So let's take a step back and analyze why the LRU lock has become a problem.
From what I can tell, it's because you want to use per-object lock when
reserving instead of a
global reservation lock (that TTM could use as the LRU lock). Is that correct?
and in that case, in what situation do you envision such a global lock being
contended
to the extent that it hurts performance?
Lockdep WILL complain about trying to use multiple tickets, doing ticketed
and unticketed blocking reservations mixed, etc.
I want to remove the global fence_lock and make it a per buffer lock, with some
lockdep annotations it's perfectly legal to grab obj->fence_lock and
obj2->fence_lock
if you have a reservation, but it should complain loudly about trying to take 2
fence_locks
at the same time without a reservation.
Yes, TTM was previously using per buffer fence locks, and that works fine from
a deadlock perspective, but
it hurts performance. Fencing 200 buffers in a command submission (google-earth
for example) will mean
198 unnecessary locks, each discarding the processor pipelines. Locking is a
*slow* operation, particularly
on systems with many processors, and I don't think it's a good idea to change
that back, without analyzing
the performance impact. There are reasons people are writing stuff like RCU to
avoid locking...
So why don't we simply use RCU for fence pointers and get rid of the fence
locking? :D
danvet originally suggested it as a joke but if you think about it, it would
make a lot of sense for this usecase.
I thought of that before, but the problem is you'd still need a spinlock to
change the buffer's fence pointer,
even if reading it becomes quick.
Actually, I changed lockdep annotations a bit to distinguish between the
cases where ttm_bo_wait is called without reservation, and ttm_bo_wait
is called with, as far as I can see there are only 2 places that do it without,
at least if I converted my git tree properly..
http://cgit.freedesktop.org/~mlankhorst/linux/log/?h=v10-wip
First one is nouveau_bo_vma_del, this can be fixed easily.
Second one is ttm_bo_cleanup_refs and ttm_bo_cleanup_refs_or_queue,
if reservation is done first before ttm_bo_wait, the fence_lock could be
dropped entirely by adding smb_mb() in reserve and unreserve, functionally
there would be no difference. So if you can verify my lockdep annotations are
correct in the most recent commit wrt what's using ttm_bo_wait without
reservation
we could remove the fence_lock entirely.
~Maarten
Being able to wait for buffer idle or get the fence pointer without
reserving is a fundamental property of TTM. Reservation is a long-term
lock. The fence lock is a very short term lock. If I were to choose, I'd
rather accept per-object fence locks than removing this property, but
see below.
Likewise, to be able to guarantee that a reserved object is not on any
LRU list is also an important property. Removing that property will, in
addition to the spin wait we've already discussed make understanding TTM
locking even more difficult, and I'd really like to avoid it.
If this were a real performance problem we were trying to solve it would
be easier to motivate changes in this area, but if it's just trying to
avoid a global reservation lock and a global fence lock that will rarely
if ever see any contention, I can't see the point. On the contrary,
having per-object locks will be very costly when reserving / fencing
many objects. As mentioned before, in the fence lock case it's been
tried and removed, so I'd like to know the reasoning behind introducing
it again, and in what situations you think the global locks will be
contended.
/Thomas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
