From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH v3 3/4] drm/ttm: convert to unified vma offset manager Date: Mon, 22 Jul 2013 13:45:49 +0200 Message-ID: <51ED1B6D.3010900@vmware.com> References: <1374084860-29768-1-git-send-email-dh.herrmann@gmail.com> <1374084860-29768-4-git-send-email-dh.herrmann@gmail.com> <51E7AD18.4050100@vmware.com> <51E7D080.3030405@vmware.com> <51E90330.5070702@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-1.vmware.com (smtp-outbound-1.vmware.com [208.91.2.12]) by gabe.freedesktop.org (Postfix) with ESMTP id 03D6DE62A5 for ; Mon, 22 Jul 2013 04:45:56 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: David Herrmann Cc: Martin Peres , "dri-devel@lists.freedesktop.org" , Alex Deucher , Ben Skeggs , Dave Airlie List-Id: dri-devel@lists.freedesktop.org On 07/22/2013 12:55 PM, David Herrmann wrote: > Sorry, I forgot to CC correctly. > > On Mon, Jul 22, 2013 at 12:53 PM, David Herrmann wrote: >> Hi >> >> On Fri, Jul 19, 2013 at 11:13 AM, Thomas Hellstrom >> wrote: >>> On 07/18/2013 10:54 PM, David Herrmann wrote: >>>> Hi >>>> >>>> On Thu, Jul 18, 2013 at 1:24 PM, Thomas Hellstrom >>>> wrote: >>> >>> ... >>> >>> >>>>> I think that if there are good reasons to keep locking internal, I'm fine >>>>> with that, (And also, of course, with >>>>> Daniel's proposal). Currently the add / remove / lookup paths are mostly >>>>> used by TTM during object creation and >>>>> destruction. >>>>> >>>>> However, if the lookup path is ever used by pread / pwrite, that >>>>> situation >>>>> might change and we would then like to >>>>> minimize the locking. >>>> I tried to keep the change as minimal as I could. Follow-up patches >>>> are welcome. I just thought pushing the lock into drm_vma_* would >>>> simplify things. If there are benchmarks that prove me wrong, I'll >>>> gladly spend some time optimizing that. >>> >>> In the general case, one reason for designing the locking outside of a >>> utilities like this, is that different callers may have >>> different requirements. For example, the call path is known not to be >>> multithreaded at all, or >>> the caller may prefer a mutex over a spinlock for various reasons. It might >>> also be that some callers will want to use >>> RCU locking in the future if the lookup path becomes busy, and that would >>> require *all* users to adapt to RCU object destruction... >>> >>> I haven't looked at the code closely enough to say that any of this applies >>> in this particular case, though. >> Some notes why I went with local locking: >> - mmap offset creation is done once and is independent of any other >> operations you might do on the BO in parallel >> - mmap lookup is also only done once in most cases. User-space rarely >> maps a buffer twice (setup/teardown of mmaps is expensive, but keeping >> them around is very cheap). And for shared buffers only the writer >> maps it as the reader normally passes it to the kernel without >> mapping/touching it. Only for software-rendering we have two or more >> mappings of the same object. >> - creating/mapping/destroying buffer objects is expensive in its >> current form and buffers tend to stay around for a long time >> >> I couldn't find a situation were the vma-manager was part of a >> performance critical path. But on the other hand, the paths were it is >> used are quite heavy. That's why I don't want to lock the whole buffer >> or even device. Instead, we need some "management lock" which is used >> for mmap-setup or similar things that don't affect other operations on >> the buffer or device. We don't have such a lock, so I introduced local >> locking. If we end up with more use-cases, I volunteer to refactor >> this. But I am no big fan of overgeneralizing it before more uses are >> known. I think we are discussing two different things here: 1) Having a separate lock for vma management vs 2) building that lock into the vma manager utility. You're arguing for 1) and I completely agree with you, and although I still think one generally should avoid building locks into utilities like this (avoid 2), I'm fine with the current approach. Thanks, Thomas