Hey, Op 18-10-12 09:59, Thomas Hellstrom schreef: > > > > On 10/18/2012 09:28 AM, Thomas Hellstrom wrote: >> Hi, Maarten, >> >> As you know I have been having my doubts about this change. >> To me it seems insane to be forced to read the fence pointer under a >> reserved lock, simply because when you take the reserve lock, another >> process may have it and there is a substantial chance that that process >> will also be waiting for idle while holding the reserve lock. I think it makes perfect sense, the only times you want to read the fence is when you want to change the members protected by the reservation. >> In essence, to read the fence pointer, there is a large chance you will >> be waiting for idle to be able to access it. That's why it's protected by >> a separate spinlock in the first place. >> >> So even if this change might not affect current driver much it's a change >> to the TTM api that leads to an IMHO very poor design. I would argue the opposite, no longer having a separate lock, with clear semantics when fencing is allowed, gives you a way to clean up the core of ttm immensely. There were only 2 functions affected by fence lock removal and they were on buffer destruction, ttm_bo_cleanup_refs (and *_or_queue). But since the *_or_queue can simply change order around, you only have to worry about ttm_bo_cleanup_refs. This function is already ugly for other reasons, and the followup patch I was suggesting cleaned up the ugliness there too. The only thing done differently is backing off from the reservation early. With the cleanup it won't even try to get the reservation again, since nobody should set a new fence on the bo when it's dead. Instead all destruction is moved until list refcount drops to 0. > One way we could perhaps improve on this, if you think this is necessary, is to > build a bit on the initial rcu suggestion, still require reservation when the fence pointer is modified, > but to also use rcu_assign_pointer() for the fence pointer. This is a massive overkill when the only time you read the fence pointer without reservation is during buffer destruction. RCU is only good if there's ~10x more reads than writes, and for us it's simply 50% reads 50% writes.. > Anyone that wants quick access to the fence pointer would then use rcu_dereference_x(), but > if the fence is indeed signaled, that caller needs to avoid setting the bo fence pointer to NULL. > > A check for bo idle without taking any (bo) locks would then mean imply reading the fence pointer > in this fashion, and if it's non-NULL check whether the fence is signaled. Sure it may look easier, but you add more overhead and complexity. I thought you wanted to avoid overhead in the reservation path? RCU won't be the way to do that. ~Maarten _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
