From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu Date: Tue, 20 May 2014 17:32:54 +0200 Message-ID: <537B75A6.2060701@canonical.com> References: <20140409144239.26648.57918.stgit@patser> <20140409144831.26648.79163.stgit@patser> <53465A53.1090500@vmware.com> <53466D63.8080808@canonical.com> <53467B93.3000402@vmware.com> <5346B212.8050202@canonical.com> <5347A9FD.2070706@vmware.com> <5347B4E5.6090901@canonical.com> <5347BFC9.3020503@vmware.com> <53482FF1.1090406@canonical.com> <534843EA.6060602@vmware.com> <534B9165.4000101@canonical.com> <534B921B.4080504@vmware.com> <5357A0DE.7030305@canonical.com> <537A0A5D.6090909@vmware.com> <537A1180.2010109@canonical.com> <537B7122.3000508@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <537B7122.3000508@vmware.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thomas Hellstrom Cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, ccross@google.com, linux-media@vger.kernel.org List-Id: linux-arch.vger.kernel.org op 20-05-14 17:13, Thomas Hellstrom schreef: > On 05/19/2014 03:13 PM, Maarten Lankhorst wrote: >> op 19-05-14 15:42, Thomas Hellstrom schreef: >>> Hi, Maarten! >>> >>> Some nitpicks, and that krealloc within rcu lock still worries me. >>> Otherwise looks good. >>> >>> /Thomas >>> >>> >>> >>> On 04/23/2014 12:15 PM, Maarten Lankhorst wrote: >>>> @@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct >>>> reservation_object *obj) >>>> kfree(obj->staged); >>>> obj->staged = NULL; >>>> return 0; >>>> - } >>>> - max = old->shared_max * 2; >>>> + } else >>>> + max = old->shared_max * 2; >>> Perhaps as a separate reformatting patch? >> I'll fold it in to the patch that added >> reservation_object_reserve_shared. >>>> + >>>> +int reservation_object_get_fences_rcu(struct reservation_object *obj, >>>> + struct fence **pfence_excl, >>>> + unsigned *pshared_count, >>>> + struct fence ***pshared) >>>> +{ >>>> + unsigned shared_count = 0; >>>> + unsigned retry = 1; >>>> + struct fence **shared = NULL, *fence_excl = NULL; >>>> + int ret = 0; >>>> + >>>> + while (retry) { >>>> + struct reservation_object_list *fobj; >>>> + unsigned seq; >>>> + >>>> + seq = read_seqcount_begin(&obj->seq); >>>> + >>>> + rcu_read_lock(); >>>> + >>>> + fobj = rcu_dereference(obj->fence); >>>> + if (fobj) { >>>> + struct fence **nshared; >>>> + >>>> + shared_count = ACCESS_ONCE(fobj->shared_count); >>> ACCESS_ONCE() shouldn't be needed inside the seqlock? >> Yes it is, shared_count may be increased, leading to potential >> different sizes for krealloc and memcpy >> if the ACCESS_ONCE is removed. I could use shared_max here instead, >> which stays the same, >> but it would waste more memory. > Maarten, Another perhaps ignorant question WRT this, > Does ACCESS_ONCE() guarantee that the value accessed is read atomically? Well I've reworked the code to use shared_max, so this point is moot. :-) On any archs I'm aware of it would work, either the old or new value would be visible, as long as natural alignment is used. rcu uses the same trick in the rcu_dereference macro, so if this didn't work rcu wouldn't work either. ~Maarten From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from youngberry.canonical.com ([91.189.89.112]:41945 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751266AbaETPc5 (ORCPT ); Tue, 20 May 2014 11:32:57 -0400 Message-ID: <537B75A6.2060701@canonical.com> Date: Tue, 20 May 2014 17:32:54 +0200 From: Maarten Lankhorst MIME-Version: 1.0 Subject: Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu References: <20140409144239.26648.57918.stgit@patser> <20140409144831.26648.79163.stgit@patser> <53465A53.1090500@vmware.com> <53466D63.8080808@canonical.com> <53467B93.3000402@vmware.com> <5346B212.8050202@canonical.com> <5347A9FD.2070706@vmware.com> <5347B4E5.6090901@canonical.com> <5347BFC9.3020503@vmware.com> <53482FF1.1090406@canonical.com> <534843EA.6060602@vmware.com> <534B9165.4000101@canonical.com> <534B921B.4080504@vmware.com> <5357A0DE.7030305@canonical.com> <537A0A5D.6090909@vmware.com> <537A1180.2010109@canonical.com> <537B7122.3000508@vmware.com> In-Reply-To: <537B7122.3000508@vmware.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Thomas Hellstrom Cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, ccross@google.com, linux-media@vger.kernel.org Message-ID: <20140520153254.Kr_GTHC6BsR5V9FB9PB2E7cO9TkyAH0UWpplDgYwo_0@z> op 20-05-14 17:13, Thomas Hellstrom schreef: > On 05/19/2014 03:13 PM, Maarten Lankhorst wrote: >> op 19-05-14 15:42, Thomas Hellstrom schreef: >>> Hi, Maarten! >>> >>> Some nitpicks, and that krealloc within rcu lock still worries me. >>> Otherwise looks good. >>> >>> /Thomas >>> >>> >>> >>> On 04/23/2014 12:15 PM, Maarten Lankhorst wrote: >>>> @@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct >>>> reservation_object *obj) >>>> kfree(obj->staged); >>>> obj->staged = NULL; >>>> return 0; >>>> - } >>>> - max = old->shared_max * 2; >>>> + } else >>>> + max = old->shared_max * 2; >>> Perhaps as a separate reformatting patch? >> I'll fold it in to the patch that added >> reservation_object_reserve_shared. >>>> + >>>> +int reservation_object_get_fences_rcu(struct reservation_object *obj, >>>> + struct fence **pfence_excl, >>>> + unsigned *pshared_count, >>>> + struct fence ***pshared) >>>> +{ >>>> + unsigned shared_count = 0; >>>> + unsigned retry = 1; >>>> + struct fence **shared = NULL, *fence_excl = NULL; >>>> + int ret = 0; >>>> + >>>> + while (retry) { >>>> + struct reservation_object_list *fobj; >>>> + unsigned seq; >>>> + >>>> + seq = read_seqcount_begin(&obj->seq); >>>> + >>>> + rcu_read_lock(); >>>> + >>>> + fobj = rcu_dereference(obj->fence); >>>> + if (fobj) { >>>> + struct fence **nshared; >>>> + >>>> + shared_count = ACCESS_ONCE(fobj->shared_count); >>> ACCESS_ONCE() shouldn't be needed inside the seqlock? >> Yes it is, shared_count may be increased, leading to potential >> different sizes for krealloc and memcpy >> if the ACCESS_ONCE is removed. I could use shared_max here instead, >> which stays the same, >> but it would waste more memory. > Maarten, Another perhaps ignorant question WRT this, > Does ACCESS_ONCE() guarantee that the value accessed is read atomically? Well I've reworked the code to use shared_max, so this point is moot. :-) On any archs I'm aware of it would work, either the old or new value would be visible, as long as natural alignment is used. rcu uses the same trick in the rcu_dereference macro, so if this didn't work rcu wouldn't work either. ~Maarten