From: Saeed Mahameed <saeedm@dev.mellanox.co.il>
To: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
John Stultz <john.stultz@linaro.org>,
Stephen Boyd <sboyd@codeaurora.org>,
Chris Wilson <chris@chris-wilson.co.uk>,
linux-kernel <linux-kernel@vger.kernel.org>,
Linux Netdev List <netdev@vger.kernel.org>,
linux-rdma@vger.kernel.org
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]
Date: Thu, 30 Nov 2017 13:03:34 -0800 [thread overview]
Message-ID: <CALzJLG9JXOnr3EQ2zLcmwKx8S9-CGONRRBSAd9XwHdemEgOn2A@mail.gmail.com> (raw)
In-Reply-To: <63a2a495-1bdb-5d47-1202-9b538e9601d8@intel.com>
On Mon, Nov 27, 2017 at 2:05 AM, Sagar Arun Kamble
<sagar.a.kamble@intel.com> 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.
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
next prev parent reply other threads:[~2017-11-30 21:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1510748034-14034-1-git-send-email-sagar.a.kamble@intel.com>
[not found] ` <1510748034-14034-2-git-send-email-sagar.a.kamble@intel.com>
[not found] ` <151074872901.26264.3145709294590477412@mail.alporthouse.com>
2017-11-23 7:34 ` 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] Sagar Arun Kamble
2017-11-23 18:59 ` Thomas Gleixner
2017-11-24 9:06 ` Sagar Arun Kamble
2017-11-24 13:31 ` Thomas Gleixner
2017-11-27 10:05 ` Sagar Arun Kamble
2017-11-30 21:03 ` Saeed Mahameed [this message]
2017-12-01 7:42 ` Sagar Arun Kamble
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALzJLG9JXOnr3EQ2zLcmwKx8S9-CGONRRBSAd9XwHdemEgOn2A@mail.gmail.com \
--to=saeedm@dev.mellanox.co.il \
--cc=chris@chris-wilson.co.uk \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=sagar.a.kamble@intel.com \
--cc=sboyd@codeaurora.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).