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]

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 12/1/2017 2:33 AM, Saeed Mahameed wrote:
> 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.

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