Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751687AbdLAHmO (ORCPT ); Fri, 1 Dec 2017 02:42:14 -0500 Received: from mga04.intel.com ([192.55.52.120]:33687 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751596AbdLAHmL (ORCPT ); Fri, 1 Dec 2017 02:42:11 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,344,1508828400"; d="scan'208";a="7917442" Subject: Re: Creating cyclecounter and lock member in timecounter structure [ Was Re: [RFC 1/4] drm/i915/perf: Add support to correlate GPU timestamp with system time] To: Saeed Mahameed Cc: Thomas Gleixner , John Stultz , Stephen Boyd , Chris Wilson , linux-kernel , Linux Netdev List , linux-rdma@vger.kernel.org References: <1510748034-14034-1-git-send-email-sagar.a.kamble@intel.com> <1510748034-14034-2-git-send-email-sagar.a.kamble@intel.com> <151074872901.26264.3145709294590477412@mail.alporthouse.com> <83805c37-99b2-2922-e8e8-ea435f9da7d0@intel.com> <63a2a495-1bdb-5d47-1202-9b538e9601d8@intel.com> From: Sagar Arun Kamble Message-ID: <21599c2b-a89a-4047-cff8-d56c6312cd4b@intel.com> Date: Fri, 1 Dec 2017 13:12:02 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4185 Lines: 113 On 12/1/2017 2:33 AM, Saeed Mahameed wrote: > On Mon, Nov 27, 2017 at 2:05 AM, Sagar Arun Kamble > wrote: >> >> On 11/24/2017 7:01 PM, Thomas Gleixner wrote: >>> On Fri, 24 Nov 2017, Sagar Arun Kamble wrote: >>>> On 11/24/2017 12:29 AM, Thomas Gleixner wrote: >>>>> On Thu, 23 Nov 2017, Sagar Arun Kamble wrote: >>>>>> We needed inputs on possible optimization that can be done to >>>>>> timecounter/cyclecounter structures/usage. >>>>>> This mail is in response to review of patch >>>>>> https://patchwork.freedesktop.org/patch/188448/. >>>>>> >>>>>> As Chris's observation below, about dozen of timecounter users in the >>>>>> kernel >>>>>> have below structures >>>>>> defined individually: >>>>>> >>>>>> spinlock_t lock; >>>>>> struct cyclecounter cc; >>>>>> struct timecounter tc; >>>>>> >>>>>> Can we move lock and cc to tc? That way it will be convenient. >>>>>> Also it will allow unifying the locking/overflow watchdog handling >>>>>> across >>>>>> all >>>>>> drivers. >>>>> Looks like none of the timecounter usage sites has a real need to >>>>> separate >>>>> timecounter and cyclecounter. >>>> Yes. Will share patch for this change. >>>> >>>>> The lock is a different question. The locking of the various drivers >>>>> differs and I have no idea how you want to handle that. Just sticking >>>>> the >>>>> lock into the datastructure and then not making use of it in the >>>>> timercounter code and leave it to the callsites does not make sense. >>>> Most of the locks are held around timecounter_read. In some instances it >>>> is held when cyclecounter is updated standalone or is updated along with >>>> timecounter calls. Was thinking if we move the lock in timecounter >>>> functions, drivers just have to do locking around its operations on >>>> cyclecounter. But then another problem I see is there are variation of >>>> locking calls like lock_irqsave, lock_bh, write_lock_irqsave (some using >>>> rwlock_t). Should this all locking be left to driver only then? >>> You could have the lock in the struct and protect the inner workings in >>> the >>> related core functions. >>> >>> That might remove locking requirements from some of the callers and the >>> others still have their own thing around it. >> >> For drivers having static/fixed cyclecounter, we can rely only on lock >> inside timecounter. >> Most of the network drivers update cyclecounter at runtime and they will >> have to rely on two locks if >> we add one to timecounter. This may not be efficient for them. Also the lock >> in timecounter has to be less restrictive (may be seqlock) I guess. >> >> Cc'd Mellanox list for inputs on this. >> >> I have started feeling that the current approach of drivers managing the >> locks is the right one so better leave the >> lock out of timecounter. >> > I agree here, > > In mlx5 we rely on our own read/write lock to serialize access to > mlx5_clock struct (mlx5 timecounter and cyclecounter). > the access is not as simple as > lock() > call time_counter_API > unlock() > > Sometimes we also explicitly update/adjust timecycles counters with > mlx5 specific calculations after we read the timecounter all inside > our lock. > e.g. > @mlx5_ptp_adjfreq() > > write_lock_irqsave(&clock->lock, flags); > timecounter_read(&clock->tc); > clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff : > clock->nominal_c_mult + diff; > write_unlock_irqrestore(&clock->lock, flags); > > So i don't think it will be a simple task to have a generic thread > safe timecounter API, without the need to specifically adjust it for > all driver use-cases. > Also as said above, in runtime it is not obvious in which context the > timecounter will be accessed irq/soft irq/user. > > let's keep it as is, and let the driver decide which locking scheme is > most suitable for it. Yes. Thanks for your inputs Saeed. Regards Sagar > > Thanks, > Saeed. > >>> Thanks, >>> >>> tglx >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html